feat: add MCP server support #286
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds end-to-end Model Context Protocol (MCP) support: new types, SDK dependency, settings helpers, MCP test service and routes, provider/agent option plumbing, UI for managing MCP servers, store/API surface updates, and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (Settings)
participant Store as App Store
participant Server as Server (Settings API)
participant Agent as Agent Service
participant Provider as Claude Provider
participant SDK as MCP SDK
Note over UI,Store: User updates MCP servers or flags
UI->>Store: update mcpServers / set flags
Store->>Server: syncSettingsToServer (includes mcpServers & flags)
Server-->>Store: ack
Note over Agent,Server: Request-time MCP load
Agent->>Server: getGlobal settings
Server-->>Agent: { mcpServers, mcpAutoApproveTools, mcpUnrestrictedTools }
Agent->>Provider: execute(options with mcpServers & flags)
Provider->>Provider: compute shouldRestrict / shouldBypass
alt bypassPermissions
Provider->>SDK: init with bypassPermissions + mcpServers
else restricted
Provider->>SDK: init with allowedTools + mcpServers
end
SDK-->>Provider: execution result
Provider-->>Agent: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
apps/server/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (3)apps/ui/src/components/views/settings-view/mcp-servers/dialogs/security-warning-dialog.tsx (2)
apps/server/src/lib/sdk-options.ts (1)
libs/types/src/settings.ts (1)
🔇 Additional comments (18)
Comment |
Summary of ChangesHello @mzubair481, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the AI agent's capabilities by integrating Model Context Protocol (MCP) server support. It provides a robust framework for users to configure and manage external tools that AI agents can utilize, complete with a new, intuitive settings interface. The changes ensure that agents can interact with a broader range of functionalities while offering granular control over tool permissions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Model Context Protocol (MCP) servers, allowing AI agents to be extended with external tools. The changes include a new UI section for managing MCP servers, logic for handling permissions and tool access when MCP servers are configured, and updates to various types and services to support this new functionality. The implementation is comprehensive, covering both backend services and the user interface. My review focuses on code duplication, potential inconsistencies in option handling, and minor improvements for robustness and developer experience.
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/lib/http-api-client.ts (1)
889-943: Add MCP permission flags tosettings.getGlobalreturn type
loadMCPServersFromServerreadsmcpAutoApproveToolsandmcpUnrestrictedToolsfromresult.settings, but thegetGlobaltype here only exposesmcpServers. This will either break TypeScript or force unsafe casts, and it drifts from the sharedGlobalSettingsshape.Consider extending the inline
settingstype to include these flags:Proposed type update
lastSelectedSessionByProject: Record<string, string>; - mcpServers?: Array<{ + mcpServers?: Array<{ id: string; name: string; description?: string; type?: 'stdio' | 'sse' | 'http'; command?: string; args?: string[]; env?: Record<string, string>; url?: string; headers?: Record<string, string>; enabled?: boolean; - }>; + }>; + mcpAutoApproveTools: boolean; + mcpUnrestrictedTools: boolean; };apps/ui/src/store/app-store.ts (1)
2737-2793: Persist MCP settings in Zustand to avoid losing them between sessions (especially in web mode)
mcpServers,mcpAutoApproveTools, andmcpUnrestrictedToolsare part ofAppStateand initialState, but they’re not included in thepartializeblock used bypersist. That means:
- In Electron, they’re only coming back from the server settings file, not from the local cache.
- In web mode (where
syncSettingsToServeris a no‑op), MCP configuration will be lost on page reload.To keep MCP settings durable across reloads in all environments, add them to the persisted slice:
Proposed `partialize` update
partialize: (state) => ({ // Project management projects: state.projects, currentProject: state.currentProject, trashedProjects: state.trashedProjects, projectHistory: state.projectHistory, projectHistoryIndex: state.projectHistoryIndex, // Features - cached locally for faster hydration (authoritative source is server) features: state.features, // UI state currentView: state.currentView, theme: state.theme, sidebarOpen: state.sidebarOpen, chatHistoryOpen: state.chatHistoryOpen, kanbanCardDetailLevel: state.kanbanCardDetailLevel, boardViewMode: state.boardViewMode, // Settings apiKeys: state.apiKeys, maxConcurrency: state.maxConcurrency, defaultSkipTests: state.defaultSkipTests, enableDependencyBlocking: state.enableDependencyBlocking, useWorktrees: state.useWorktrees, currentWorktreeByProject: state.currentWorktreeByProject, worktreesByProject: state.worktreesByProject, showProfilesOnly: state.showProfilesOnly, keyboardShortcuts: state.keyboardShortcuts, muteDoneSound: state.muteDoneSound, enhancementModel: state.enhancementModel, validationModel: state.validationModel, autoLoadClaudeMd: state.autoLoadClaudeMd, enableSandboxMode: state.enableSandboxMode, + mcpServers: state.mcpServers, + mcpAutoApproveTools: state.mcpAutoApproveTools, + mcpUnrestrictedTools: state.mcpUnrestrictedTools, // Profiles and sessions aiProfiles: state.aiProfiles, chatSessions: state.chatSessions, lastSelectedSessionByProject: state.lastSelectedSessionByProject, // Board background settings boardBackgroundByProject: state.boardBackgroundByProject, // Terminal layout persistence (per-project) terminalLayoutByProject: state.terminalLayoutByProject, // Terminal settings persistence (global) terminalSettings: { defaultFontSize: state.terminalState.defaultFontSize, defaultRunScript: state.terminalState.defaultRunScript, screenReaderMode: state.terminalState.screenReaderMode, fontFamily: state.terminalState.fontFamily, scrollbackLines: state.terminalState.scrollbackLines, lineHeight: state.terminalState.lineHeight, maxSessions: state.terminalState.maxSessions, } as PersistedTerminalSettings, defaultPlanningMode: state.defaultPlanningMode, defaultRequirePlanApproval: state.defaultRequirePlanApproval, defaultAIProfileId: state.defaultAIProfileId, }) as any,
🧹 Nitpick comments (8)
libs/types/src/provider.ts (1)
69-71: Security consideration: Document the implications of MCP permission settings.The
mcpAutoApproveToolsandmcpUnrestrictedToolsflags have security implications, as they control whether MCP tool calls require approval and whether tool restrictions apply.Based on the library documentation context, these settings affect the
permissionModeconfiguration in the Claude SDK, potentially usingbypassPermissionsmode which skips permission prompts.Consider adding JSDoc comments that explicitly warn about the security implications:
- mcpAutoApproveTools?: boolean; // Auto-approve MCP tool calls without permission prompts - mcpUnrestrictedTools?: boolean; // Allow unrestricted tools when MCP servers are enabled + /** + * Auto-approve MCP tool calls without permission prompts. + * WARNING: When true, uses SDK's bypassPermissions mode which skips all permission checks. + * Only enable in trusted environments. + */ + mcpAutoApproveTools?: boolean; + /** + * Allow unrestricted tools when MCP servers are enabled. + * When false, restricts tools to the predefined allowedTools list even with MCP servers. + */ + mcpUnrestrictedTools?: boolean;libs/types/src/settings.ts (1)
186-211: Document the relationship between MCPServerConfig and provider types.
MCPServerConfig(settings-facing) andMcpServerConfig(provider-facing) have structural differences:
- Settings version includes UI-specific fields:
enabled,tools,toolsLastFetched- Settings version has all fields optional (e.g.,
command?,url?)- Provider version requires discriminant fields for proper type narrowing
Consider adding a JSDoc comment explaining the conversion requirements:
/** * MCPServerConfig - Configuration for an MCP (Model Context Protocol) server * * MCP servers provide additional tools and capabilities to AI agents. * Supports stdio (subprocess), SSE, and HTTP transport types. * * Note: This is the UI/settings representation. When passed to the provider, * it's converted to McpServerConfig (see libs/types/src/provider.ts) which * omits UI-specific fields (enabled, tools, toolsLastFetched) and enforces * stricter type requirements. */ export interface MCPServerConfig {apps/server/src/lib/sdk-options.ts (1)
234-239: Consider consolidating MCP type exports.The MCP types are re-exported here for convenience, but they're also exported from
@automaker/types. This creates multiple import paths for the same types.Consider whether these re-exports are necessary. If consumers can import directly from
@automaker/types, these re-exports might be redundant:// Current: Multiple import paths import { McpServerConfig } from '@automaker/types'; import { McpServerConfig } from 'apps/server/src/lib/sdk-options'; // Simpler: Single source of truth import { McpServerConfig } from '@automaker/types';If the re-exports are kept, add a JSDoc comment explaining why:
// Re-export MCP types from @automaker/types for convenience // These are used in CreateSdkOptionsConfig and related functions export type { McpServerConfig, ... } from '@automaker/types';apps/server/src/services/auto-mode-service.ts (1)
35-41: Auto‑mode MCP wiring matches AgentService pattern
runAgentnow loads MCP servers and permissions, feeds them intocreateAutoModeOptions, and forwards them viaExecuteOptionsonly when servers exist. This keeps the auto‑mode pipeline aligned with the interactive agent path and preserves prior semantics when MCP is not configured.You could later cache MCP settings per service instance if repeated settings reads become a hotspot, but it’s not required for this PR.
Also applies to: 1846-1862, 1893-1906
apps/ui/src/hooks/use-settings-migration.ts (1)
20-25: MCP sync and reload helpers are consistent with store shapeExtending
syncSettingsToServerto includemcpServers,mcpAutoApproveTools, andmcpUnrestrictedTools, plus the newloadMCPServersFromServerhelper that refreshes these into Zustand, lines up with the new MCP state inapp-store.ts. The Electron guard and default‑to‑true flags match the server‑side defaults.Once
HttpApiClient.settings.getGlobalis updated to type these MCP fields, this flow should be type‑safe end‑to‑end.Also applies to: 211-243, 324-361
apps/ui/src/store/app-store.ts (1)
4-11: MCP state and actions are wired correctly into the store
- AppState and initialState for
mcpServers,mcpAutoApproveTools, andmcpUnrestrictedToolsmatch the sharedMCPServerConfigshape and server defaults.- Flag setters mirror
setAutoLoadClaudeMd/setEnableSandboxModeby updating state then delegating persistence tosyncSettingsToServer.- MCP server CRUD and reordering logic use the usual immutable array patterns and generate stable IDs.
If you want MCP server list changes to always hit the settings file without relying on the settings UI to call
syncSettingsToServerexplicitly, consider invoking it fromaddMCPServer/updateMCPServer/removeMCPServer/reorderMCPServersas well.Also applies to: 486-490, 767-769, 777-781, 949-951, 1590-1601, 1643-1664
apps/server/src/lib/settings-helpers.ts (1)
222-246: Non-null assertions on optional fields without validation.The
convertToSdkFormatfunction uses non-null assertions (!) onserver.urlandserver.commandwithout verifying these fields exist. While the caller (getMCPServersFromSettings) doesn't validate these fields either, runtime errors could occur if a malformed config is stored.🔎 Proposed defensive handling
function convertToSdkFormat(server: MCPServerConfig): McpServerConfig { if (server.type === 'sse') { + if (!server.url) { + throw new Error(`SSE server "${server.name}" is missing required 'url' field`); + } return { type: 'sse', - url: server.url!, + url: server.url, headers: server.headers, }; } if (server.type === 'http') { + if (!server.url) { + throw new Error(`HTTP server "${server.name}" is missing required 'url' field`); + } return { type: 'http', - url: server.url!, + url: server.url, headers: server.headers, }; } // Default to stdio + if (!server.command) { + throw new Error(`Stdio server "${server.name}" is missing required 'command' field`); + } return { type: 'stdio', - command: server.command!, + command: server.command, args: server.args, env: server.env, }; }apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
78-81: Missing dependency and no error handling for initial load.The
useEffectcallsloadMCPServersFromServer()but doesn't handle errors or show feedback on mount failure. Additionally, ESLint rules typically flag the empty dependency array when using async functions that aren't defined inside the effect.🔎 Consider handling initial load failures
// Auto-load MCP servers from settings file on mount useEffect(() => { - loadMCPServersFromServer(); + loadMCPServersFromServer().catch((error) => { + console.error('Failed to load MCP servers on mount:', error); + }); }, []);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/server/package.jsonapps/server/src/lib/sdk-options.tsapps/server/src/lib/settings-helpers.tsapps/server/src/providers/claude-provider.tsapps/server/src/providers/types.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/mcp-servers/index.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/store/app-store.tslibs/types/src/index.tslibs/types/src/provider.tslibs/types/src/settings.ts
🧰 Additional context used
🧬 Code graph analysis (8)
libs/types/src/settings.ts (1)
libs/types/src/index.ts (2)
MCPToolInfo(61-61)MCPServerConfig(62-62)
apps/server/src/services/agent-service.ts (1)
apps/server/src/lib/settings-helpers.ts (2)
getMCPServersFromSettings(149-183)getMCPPermissionSettings(192-217)
libs/types/src/provider.ts (3)
apps/server/src/lib/sdk-options.ts (4)
McpServerConfig(235-235)McpStdioServerConfig(236-236)McpSSEServerConfig(237-237)McpHttpServerConfig(238-238)apps/server/src/providers/types.ts (4)
McpServerConfig(12-12)McpStdioServerConfig(13-13)McpSSEServerConfig(14-14)McpHttpServerConfig(15-15)libs/types/src/index.ts (4)
McpServerConfig(16-16)McpStdioServerConfig(17-17)McpSSEServerConfig(18-18)McpHttpServerConfig(19-19)
apps/server/src/lib/sdk-options.ts (3)
apps/server/src/providers/types.ts (1)
McpServerConfig(12-12)libs/types/src/index.ts (1)
McpServerConfig(16-16)libs/types/src/provider.ts (1)
McpServerConfig(35-35)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (5)
apps/ui/src/store/app-store.ts (1)
useAppStore(980-2796)libs/types/src/index.ts (1)
MCPServerConfig(62-62)libs/types/src/settings.ts (1)
MCPServerConfig(186-211)apps/ui/src/hooks/use-settings-migration.ts (2)
loadMCPServersFromServer(335-361)syncSettingsToServer(196-247)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/server/src/lib/settings-helpers.ts (3)
apps/server/src/providers/types.ts (1)
McpServerConfig(12-12)libs/types/src/provider.ts (1)
McpServerConfig(35-35)libs/types/src/settings.ts (1)
MCPServerConfig(186-211)
apps/ui/src/components/views/settings-view.tsx (2)
apps/ui/src/components/views/settings-view/mcp-servers/index.ts (1)
MCPServersSection(1-1)apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
MCPServersSection(59-639)
apps/ui/src/store/app-store.ts (2)
libs/types/src/settings.ts (1)
MCPServerConfig(186-211)apps/ui/src/hooks/use-settings-migration.ts (1)
syncSettingsToServer(196-247)
🔇 Additional comments (17)
apps/ui/src/components/views/context-view.tsx (1)
181-186: LGTM! Ensures metadata file exists for external tool compatibility.The logic correctly initializes the metadata file with the proper default structure if it's missing. While
loadMetadata()already handles missing files gracefully in-memory, this change ensures the file physically exists on disk—likely necessary for external MCP servers or other tools to reliably access context metadata.apps/ui/src/components/views/settings-view.tsx (1)
120-121: LGTM! MCP Servers section integration is clean.The integration follows the established pattern for settings sections. The component is self-contained and manages its own state through the app store, consistent with other settings sections.
apps/ui/src/components/views/settings-view/config/navigation.ts (1)
12-12: LGTM! Navigation item addition is well-chosen.The
Plugicon is semantically appropriate for MCP (Model Context Protocol) server connections, and the positioning after the Claude section makes logical sense since MCP extends Claude's capabilities.Also applies to: 26-26
apps/ui/src/components/views/settings-view/mcp-servers/index.ts (1)
1-1: LGTM! Clean barrel export.Standard barrel export pattern that enables cleaner imports. Well-structured.
apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts (1)
6-6: LGTM! SettingsViewId extension is correct.The addition of
'mcp-servers'to the union type enables the new MCP Servers settings view. The positioning after'claude'is logical since MCP extends Claude's capabilities.apps/server/package.json (1)
31-31: Package version is valid and available on npm. The @modelcontextprotocol/sdk@1.25.1 package exists and is the latest version with no reported security vulnerabilities.libs/types/src/index.ts (1)
6-20: MCP type re‑exports look consistentRe‑exporting the MCP provider and settings types here is a clean way to centralize the public surface; it keeps server/provider code and UI using the same definitions without duplicating shapes.
Also applies to: 52-71
apps/server/src/providers/claude-provider.ts (1)
38-49: MCP permission and tool‑restriction logic is coherentThe derived flags (
hasMcpServers,shouldBypassPermissions,shouldRestrictTools) correctly gate:
- when to bypass permissions and set
allowDangerouslySkipPermissions- when to omit
allowedTools(unrestricted MCP) vs pass explicit/default tools- when to forward
mcpServersintosdkOptionsThis matches the intent described in the PR and should preserve previous behavior when no MCP servers are configured.
Also applies to: 55-62, 71-73
apps/server/src/services/agent-service.ts (1)
20-26: MCP settings flow through AgentService correctlyMCP servers and permission flags are loaded once per send, threaded into
createChatOptions, and forwarded to the provider viaExecuteOptionsonly when non‑empty. This keeps the existing flow intact while enabling MCP, and relies on centralized helpers for defaults and logging.Also applies to: 232-237, 255-266, 277-292
apps/server/src/providers/types.ts (1)
1-16: Consolidating provider types via@automaker/typeslooks goodRe‑exporting the core provider/MCP types from
@automaker/typesand keeping only the stream‑specific interfaces local reduces duplication and keeps ExecuteOptions/MCP server shapes in sync across server and UI.apps/server/src/lib/settings-helpers.ts (2)
149-183: LGTM! Solid error handling and filtering logic.The function correctly filters enabled servers, converts to SDK format, provides informative logging, and gracefully returns an empty object on errors instead of throwing—appropriate for a settings helper that shouldn't crash the service.
192-217: LGTM! Consistent default handling and error recovery.The permission settings function follows the same pattern as other helpers in this file, with sensible defaults (
truefor backwards compatibility) and graceful error handling.apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (5)
124-166: LGTM! Solid form validation and conditional field handling.The save handler correctly validates required fields based on server type (command for stdio, URL for SSE/HTTP), preserves
enabledstate when editing, and properly splits args by whitespace.
186-268: LGTM! Robust import handling with good UX feedback.The import function handles both Claude Code format (
{ mcpServers: {...} }) and direct format, validates server configs per type, skips duplicates gracefully, and provides differentiated toast feedback for success/skipped/empty cases.
270-294: LGTM! Clean export format matching Claude Code compatibility.The export correctly builds the
{ mcpServers: {...} }structure, includes only relevant fields per server type, and uses clipboard API with success feedback.
296-470: LGTM! Well-structured UI with proper accessibility attributes.The component uses semantic structure, proper labels,
data-testidattributes for testing, and appropriate visual states for enabled/disabled servers.
168-172: Toast message is inverted.The toast displays
'Server disabled'whenserver.enabledistrue, but at this point the server hasn't been toggled yet—updateMCPServeris called on Line 169 after reading the current state. The message should reflect the new state, not the old one.🔎 Proposed fix
const handleToggleEnabled = async (server: MCPServerConfig) => { updateMCPServer(server.id, { enabled: !server.enabled }); await syncSettingsToServer(); - toast.success(server.enabled ? 'Server disabled' : 'Server enabled'); + toast.success(!server.enabled ? 'Server enabled' : 'Server disabled'); };Likely an incorrect or invalid review comment.
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
Outdated
Show resolved
Hide resolved
mzubair481
force-pushed
the
feature/mcp-server-support
branch
from
9b56ac1 to
95c1422
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/server/src/providers/claude-provider.ts (1)
39-64: Code duplication acknowledged but not resolved.The comment at lines 39-41 acknowledges that this logic mirrors
buildMcpOptions()insdk-options.ts. While the comment explains why it's here, consider extracting shared MCP permission computation to a utility function to avoid drift between the two implementations.The current approach works but requires maintaining identical logic in two places.
libs/types/src/provider.ts (1)
42-47: Type discriminant inconsistency remains.As noted in previous reviews,
McpStdioServerConfig.typeis optional whileMcpSSEServerConfig.typeandMcpHttpServerConfig.typeare required. This breaks TypeScript's discriminated union narrowing.The comment explains the design intent (simpler configs), but type consumers will need runtime checks instead of type narrowing:
// This won't narrow correctly: if (config.type === 'stdio') { /* ... */ } // Must use: if (config.type === 'stdio' || !config.type) { /* ... */ }Consider making
typerequired and handling defaults at the config loading layer instead.apps/server/src/lib/sdk-options.ts (1)
161-164: Security: Permissive defaults bypass all permission checks.The defaults (
mcpAutoApprove: true,mcpUnrestricted: true) combined with the bypass logic at lines 174-179 mean that simply adding an MCP server will:
- Set
permissionMode: 'bypassPermissions'- Enable
allowDangerouslySkipPermissions: true- Remove tool restrictions
While the comment correctly describes this as a "deliberate design choice for ease of use," users may not realize adding an MCP server disables security guardrails.
Consider one of:
- Adding a runtime warning when bypass is enabled
- Documenting this prominently in the MCP settings UI
- Defaulting to
falseand requiring explicit opt-inapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
149-150: Argument parsing doesn't handle quoted strings.The current implementation
.split(/\s+/)breaks arguments containing spaces (e.g., paths like/path/with spaces). This was noted in a previous review.For proper handling, consider a shell-like argument parser or document the limitation:
🔎 Simple quoted-string-aware parser
function parseArgs(input: string): string[] { const args: string[] = []; let current = ''; let inQuotes = false; let quoteChar = ''; for (const char of input) { if ((char === '"' || char === "'") && !inQuotes) { inQuotes = true; quoteChar = char; } else if (char === quoteChar && inQuotes) { inQuotes = false; quoteChar = ''; } else if (char === ' ' && !inQuotes) { if (current) args.push(current); current = ''; } else { current += char; } } if (current) args.push(current); return args; }
🧹 Nitpick comments (2)
apps/server/src/services/auto-mode-service.ts (1)
1846-1851: Consider caching MCP settings to reduce I/O overhead.The MCP servers and permissions are loaded from settings on every agent execution in
runAgent. Since these are global settings that don't change frequently during execution, consider caching them at the service level and invalidating the cache only when settings are updated. This would reduce file I/O overhead, especially during auto-mode with multiple concurrent features.💡 Potential caching approach
// In AutoModeService class private mcpSettingsCache: { servers: Record<string, McpServerConfig>; permissions: { mcpAutoApproveTools: boolean; mcpUnrestrictedTools: boolean }; lastLoaded: number; } | null = null; private async getMCPSettings(): Promise<{ servers: Record<string, McpServerConfig>; permissions: { mcpAutoApproveTools: boolean; mcpUnrestrictedTools: boolean }; }> { // Cache for 30 seconds if (this.mcpSettingsCache && Date.now() - this.mcpSettingsCache.lastLoaded < 30000) { return { servers: this.mcpSettingsCache.servers, permissions: this.mcpSettingsCache.permissions, }; } const servers = await getMCPServersFromSettings(this.settingsService, '[AutoMode]'); const permissions = await getMCPPermissionSettings(this.settingsService, '[AutoMode]'); this.mcpSettingsCache = { servers, permissions, lastLoaded: Date.now(), }; return { servers, permissions }; } // Then in runAgent: const { servers: mcpServers, permissions: mcpPermissions } = await this.getMCPSettings();apps/server/src/services/agent-service.ts (1)
232-236: Consider caching MCP settings to reduce I/O overhead.Similar to auto-mode-service, the MCP settings are loaded on every message send. Since these are global settings, consider caching them at the service level with appropriate cache invalidation to reduce file I/O overhead during active chat sessions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/server/package.jsonapps/server/src/lib/sdk-options.tsapps/server/src/lib/settings-helpers.tsapps/server/src/providers/claude-provider.tsapps/server/src/providers/types.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/mcp-servers/index.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/store/app-store.tslibs/types/src/index.tslibs/types/src/provider.tslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/ui/src/components/views/context-view.tsx
- libs/types/src/settings.ts
- apps/ui/src/components/views/settings-view/mcp-servers/index.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/server/src/services/auto-mode-service.ts (2)
apps/server/src/lib/settings-helpers.ts (2)
getMCPServersFromSettings(149-183)getMCPPermissionSettings(192-217)apps/server/src/lib/sdk-options.ts (1)
createAutoModeOptions(431-460)
apps/server/src/services/agent-service.ts (1)
apps/server/src/lib/settings-helpers.ts (2)
getMCPServersFromSettings(149-183)getMCPPermissionSettings(192-217)
apps/ui/src/hooks/use-settings-migration.ts (3)
apps/ui/src/lib/electron.ts (1)
isElectron(655-665)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(1145-1150)apps/ui/src/store/app-store.ts (1)
useAppStore(980-2796)
apps/server/src/lib/sdk-options.ts (1)
libs/types/src/provider.ts (1)
McpServerConfig(35-35)
apps/ui/src/store/app-store.ts (2)
libs/types/src/settings.ts (1)
MCPServerConfig(186-211)apps/ui/src/hooks/use-settings-migration.ts (1)
syncSettingsToServer(196-247)
🔇 Additional comments (22)
apps/ui/src/components/views/settings-view/config/navigation.ts (1)
12-12: LGTM! Clean addition of MCP Servers navigation.The new navigation item follows the existing pattern and uses an appropriate icon (Plug) for MCP server configuration.
Also applies to: 26-26
apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts (1)
6-6: LGTM! Type extension for new MCP settings view.The addition of 'mcp-servers' to the SettingsViewId union type properly extends the type system to support the new settings view.
apps/ui/src/components/views/settings-view.tsx (1)
21-21: LGTM! Proper integration of MCP servers settings section.The MCPServersSection component is imported and rendered following the established pattern for other settings sections in the switch statement.
Also applies to: 120-121
apps/ui/src/lib/http-api-client.ts (1)
929-940: LGTM! Well-structured MCP server configuration type.The
mcpServerstype definition is comprehensive and supports the three MCP server types (stdio, SSE, HTTP) mentioned in the PR objectives. The structure appropriately uses optional fields and includes anenabledflag for easy toggling.apps/server/src/services/auto-mode-service.ts (1)
1859-1862: LGTM! Consistent MCP settings propagation.The MCP servers and permissions are properly propagated through both the SDK options and execution options. The conditional passing of
mcpServers(only when non-empty) is good defensive programming, and the permission defaults (both true) maintain backwards compatibility.Also applies to: 1903-1906
apps/server/src/services/agent-service.ts (1)
263-265: LGTM! Consistent MCP integration in agent service.The MCP settings are properly integrated into the agent message flow, following the same pattern as auto-mode-service. The conditional passing of
mcpServersand default handling of permissions ensures robust behavior.Also applies to: 289-291
apps/server/package.json (1)
31-31: Dependency version verified and secure.The @modelcontextprotocol/sdk at version 1.25.1 exists in npm registry as the latest release and has no known security vulnerabilities. The only published CVE (CVE-2025-66414) affected versions older than 1.24.0 and was patched in 1.24.0+.
apps/server/src/providers/claude-provider.ts (1)
74-75: MCP servers forwarding looks correct.The MCP servers configuration is properly forwarded to the SDK options when present.
libs/types/src/provider.ts (1)
73-75: ExecuteOptions MCP fields look correct.The new MCP-related fields (
mcpServers,mcpAutoApproveTools,mcpUnrestrictedTools) are properly typed and optional, aligning with the rest of the interface.apps/server/src/lib/sdk-options.ts (2)
159-184: Centralized MCP logic is well-structured.The
buildMcpOptionshelper effectively centralizes MCP permission logic, addressing previous duplication concerns. The separation ofshouldRestrictTools,bypassOptions, andmcpServerOptionsprovides clear semantics.
280-286: Type re-exports improve developer ergonomics.Re-exporting MCP types from
@automaker/typesprovides a convenient single import point for consumers of this module.apps/server/src/providers/types.ts (1)
7-16: Clean type consolidation.Re-exporting from
@automaker/typesestablishes a single source of truth for shared provider types while keeping provider-specific types local. Good refactoring approach.apps/ui/src/hooks/use-settings-migration.ts (1)
231-233: MCP fields properly added to settings sync.The three MCP-related fields are correctly included in the sync payload, ensuring persistence alongside other global settings.
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (3)
78-81: Auto-load on mount is appropriate.Loading MCP servers from the settings file on component mount ensures the UI reflects the current server state, especially if settings were modified externally.
370-398: Permission toggles now sync to server.The
onCheckedChangehandlers now correctly callsyncSettingsToServer()after updating state, addressing the previous review concern about persistence.
186-268: Import handling is robust.Good implementation supporting both Claude Code format (
{ "mcpServers": {...} }) and direct format. The deduplication by name, validation of required fields per server type, and informative toast messages provide a solid user experience.apps/ui/src/store/app-store.ts (3)
486-489: MCP state fields are well-structured.The state additions follow the existing patterns in the store. The boolean flags align with server-side defaults.
1590-1601: Async settings sync is consistent with existing patterns.The
setMcpAutoApproveToolsandsetMcpUnrestrictedToolsimplementations follow the same pattern assetAutoLoadClaudeMdandsetEnableSandboxMode, maintaining consistency.
1643-1664: MCP server CRUD actions are correctly implemented.The actions (
addMCPServer,updateMCPServer,removeMCPServer,reorderMCPServers) follow established patterns fromaiProfilesmanagement. The ID generation and immutable state updates are properly handled.Note:
reorderMCPServersis implemented but the UI doesn't appear to use drag-and-drop reordering yet. This is fine for future extensibility.apps/server/src/lib/settings-helpers.ts (3)
149-183:getMCPServersFromSettingsis well-implemented.The function correctly:
- Handles missing settings service gracefully
- Filters to only enabled servers
- Converts to SDK format with proper error handling
- Provides informative logging
192-217:getMCPPermissionSettingsfollows established patterns.Consistent with other settings helpers in this file, with appropriate defaults and error handling.
223-256: Validation added toconvertToSdkFormataddresses previous review.The function now validates required fields and throws informative errors (e.g.,
SSE MCP server "name" is missing a URL), making debugging configuration issues much easier. This addresses the previous review feedback about non-null assertions.
mzubair481
force-pushed
the
feature/mcp-server-support
branch
from
95c1422 to
6e33de9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
apps/ui/src/hooks/use-settings-migration.ts (1)
23-23: Remove unused import.The
setItemimport is unused in this file. Remove it to keep imports clean.-import { getItem, removeItem } from '@/lib/storage'; +import { getItem, removeItem } from '@/lib/storage';Note: This was already flagged in a previous review.
apps/server/src/providers/claude-provider.ts (1)
39-75: MCP permission logic duplicatessdk-options.tsimplementation.The permission handling logic (lines 42-51) duplicates similar logic in
apps/server/src/lib/sdk-options.ts(as noted in the comment at lines 39-41 and flagged in a previous review). While the logic appears correct, this duplication:
- Increases maintenance burden - Changes must be synchronized across both files
- Risk of divergence - The two implementations could become inconsistent over time
- Makes testing harder - Same logic needs to be verified in multiple places
Recommended approach:
Extract this logic into a shared helper function in
settings-helpers.ts:// In settings-helpers.ts export function computeMcpPermissionFlags(options: { mcpServers?: Record<string, unknown>; mcpAutoApproveTools?: boolean; mcpUnrestrictedTools?: boolean; }): { hasMcpServers: boolean; shouldBypassPermissions: boolean; shouldRestrictTools: boolean; } { const hasMcpServers = options.mcpServers && Object.keys(options.mcpServers).length > 0; const mcpAutoApprove = options.mcpAutoApproveTools ?? true; const mcpUnrestricted = options.mcpUnrestrictedTools ?? true; return { hasMcpServers, shouldBypassPermissions: hasMcpServers && mcpAutoApprove, shouldRestrictTools: !hasMcpServers || !mcpUnrestricted, }; }Then use it in both
claude-provider.tsandsdk-options.ts:const { shouldBypassPermissions, shouldRestrictTools } = computeMcpPermissionFlags(options);This ensures consistent behavior and reduces duplication.
libs/types/src/settings.ts (1)
520-522: Security consideration: Highly permissive defaults remain.Both MCP permission defaults (
mcpAutoApproveTools: trueandmcpUnrestrictedTools: true) are still set to permissive values, as noted in the previous review. While this prioritizes ease of use for the initial release:
mcpAutoApproveTools: trueenablesbypassPermissionsmode (skips all permission prompts)mcpUnrestrictedTools: trueremoves tool filtering when MCP servers are enabledThese defaults reduce security barriers for untrusted or malicious MCP servers.
Consider one of:
- Change defaults to
false(more secure, requires explicit opt-in)- Add prominent UI warnings when users enable these settings
- Add server trust levels (e.g., "trusted" servers auto-approved, "untrusted" require permission)
If keeping permissive defaults for UX reasons, ensure:
- Settings UI clearly explains security implications
- Documentation warns users about risks
- Future updates can introduce per-server trust settings
mcpServers: [], - mcpAutoApproveTools: true, - mcpUnrestrictedTools: true, + // WARNING: Permissive defaults for ease of use. Change to false for production/untrusted servers. + mcpAutoApproveTools: false, // Set true to auto-approve all MCP tool calls (bypasses permission prompts) + mcpUnrestrictedTools: false, // Set true to allow all tools when MCP enabled (removes tool filtering)apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
139-154: Arg splitting for stdio servers won’t handle quoted arguments with spaces
formData.argsis split with/\s+/:if (formData.args.trim()) { serverData.args = formData.args.trim().split(/\s+/); }This breaks for commands like:
--config "/path/with spaces/config.json"which would be split into
["--config", '"/path/with', 'spaces/config.json"'].If you expect such cases, consider either:
- Documenting that this field doesn’t support spaces in args, or
- Using a small argv parser that respects quotes/escaping for more robust behavior.
Given this is a settings UI, it’s probably fine to keep it simple, but worth being aware of the limitation.
apps/server/src/lib/sdk-options.ts (1)
139-149: MCP auto‑approve + unrestricted defaults still enable bypassPermissions by default
buildMcpOptionsnow centralizes the MCP logic, but the behavior is still highly permissive:
mcpAutoApproveandmcpUnrestrictedboth default totrue.- When any MCP server is configured,
shouldBypassPermissionsbecomes true, and you set:
permissionMode: 'bypassPermissions'allowDangerouslySkipPermissions: trueThis effectively disables permission prompts and explicitly opts into the “dangerous” SDK mode whenever an MCP server exists, unless users discover and toggle the settings.
Even though the comment now frames this as a deliberate ease‑of‑use choice, the risk is the same as previously flagged: tools (including MCP ones) can run without explicit user approval, which is especially concerning for less technical users or shared environments.
Consider tightening this in one of these ways:
- Default to secure values and require explicit opt‑in:
const mcpAutoApprove = config.mcpAutoApproveTools ?? false; const mcpUnrestricted = config.mcpUnrestrictedTools ?? false;
- Or, if you must keep permissive defaults, add a prominent runtime warning when bypass is active:
if (shouldBypassPermissions) { console.warn( '[MCP] Auto-approve enabled: permissionMode=bypassPermissions and allowDangerouslySkipPermissions=true. ' + 'Use only in trusted environments.' ); }and make sure this is clearly documented in settings/UX so users understand the implications.
Also applies to: 159-183, 431-459
🧹 Nitpick comments (3)
apps/server/src/lib/sdk-options.ts (1)
159-183: Verify createCustomOptions’ new MCP‑aware allowedTools behavior and simplify hasMcpServers typingTwo related points here:
- Behavior change in
createCustomOptionswhenallowedToolsis omittedconst effectiveAllowedTools = config.allowedTools ? [...config.allowedTools] : mcpOptions.shouldRestrictTools ? [...TOOL_PRESETS.readOnly] : undefined;Previously, calls to
createCustomOptionswithoutallowedToolswould leaveallowedToolsundefined (full tool set). Now, when there are no MCP servers ormcpUnrestrictedToolsis false, you default toreadOnlytools instead.That’s a good security baseline, but it’s a behavior change. Please double‑check all current
createCustomOptionscall sites that don’t passallowedTools(if any beyondanalyzeProject) to ensure they don’t expect write/Bash tools by default.
- Minor type/clarity nit:
hasMcpServersshould be booleanRight now:
const hasMcpServers = config.mcpServers && Object.keys(config.mcpServers).length > 0;This infers a
Record | booleantype, which then flows intoshouldBypassPermissions. For clarity and stricter typing, consider:const hasMcpServers = !!config.mcpServers && Object.keys(config.mcpServers).length > 0;or even:
const hasMcpServers = !!config.mcpServers && Object.keys(config.mcpServers).length > 0;so all downstream logic (
shouldRestrictTools,shouldBypassPermissions) is unambiguously boolean.Also applies to: 468-504
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
372-399: Permission toggles are syncing settings twice per changeHere, each
Switch:onCheckedChange={async (checked) => { setMcpAutoApproveTools(checked); await syncSettingsToServer(); }}calls
syncSettingsToServer()directly, but inapp-store.tsthesetMcpAutoApproveTools/setMcpUnrestrictedToolsactions already perform an asyncsyncSettingsToServer()after updating state.Result: every toggle causes two back‑to‑back syncs.
You can simplify to rely on the store actions only:
<Switch id="mcp-auto-approve" checked={mcpAutoApproveTools} onCheckedChange={setMcpAutoApproveTools} data-testid="mcp-auto-approve-toggle" /> <Switch id="mcp-unrestricted" checked={mcpUnrestrictedTools} onCheckedChange={setMcpUnrestrictedTools} data-testid="mcp-unrestricted-toggle" />This keeps the UI thinner and avoids redundant server updates while still persisting changes.
apps/ui/src/store/app-store.ts (1)
486-490: MCP store wiring looks consistent; note that setMcp already handle server sync*The MCP additions here are cohesive:
- State:
mcpServers,mcpAutoApproveTools,mcpUnrestrictedToolswith sensible placement inAppStateand persistence viapartialize.- Actions:
setMcpAutoApproveTools/setMcpUnrestrictedToolsupdate state and dynamically importsyncSettingsToServer()to persist to the settings file.addMCPServer/updateMCPServer/removeMCPServer/reorderMCPServersmutate themcpServerslist as expected.- Defaults: match the rest of the MCP stack (helpers/UI).
Given these actions already perform the sync, UI components (like
MCPServersSection) don’t need to callsyncSettingsToServeragain after invokingsetMcp*; simplifying the UI will avoid redundant syncs without changing behavior.Also applies to: 767-782, 949-952, 1590-1601, 1643-1664, 2771-2774
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/server/package.jsonapps/server/src/lib/sdk-options.tsapps/server/src/lib/settings-helpers.tsapps/server/src/providers/claude-provider.tsapps/server/src/providers/types.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/mcp-servers/index.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/store/app-store.tslibs/types/src/index.tslibs/types/src/provider.tslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/ui/src/components/views/settings-view/config/navigation.ts
- libs/types/src/index.ts
- libs/types/src/provider.ts
- apps/server/src/providers/types.ts
- apps/server/package.json
- apps/server/src/services/agent-service.ts
- apps/ui/src/components/views/settings-view/mcp-servers/index.ts
🧰 Additional context used
🧬 Code graph analysis (6)
apps/ui/src/hooks/use-settings-migration.ts (3)
apps/ui/src/lib/electron.ts (1)
isElectron(655-665)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(1147-1152)apps/ui/src/store/app-store.ts (1)
useAppStore(980-2800)
apps/server/src/lib/sdk-options.ts (3)
apps/server/src/providers/types.ts (1)
McpServerConfig(12-12)libs/types/src/index.ts (1)
McpServerConfig(16-16)libs/types/src/provider.ts (1)
McpServerConfig(35-35)
apps/server/src/services/auto-mode-service.ts (2)
apps/server/src/lib/settings-helpers.ts (2)
getMCPServersFromSettings(149-183)getMCPPermissionSettings(192-217)apps/server/src/lib/sdk-options.ts (1)
createAutoModeOptions(431-460)
apps/server/src/lib/settings-helpers.ts (2)
libs/types/src/provider.ts (1)
McpServerConfig(35-35)libs/types/src/settings.ts (1)
MCPServerConfig(186-211)
apps/ui/src/store/app-store.ts (2)
libs/types/src/settings.ts (1)
MCPServerConfig(186-211)apps/ui/src/hooks/use-settings-migration.ts (1)
syncSettingsToServer(196-247)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (3)
apps/ui/src/store/app-store.ts (1)
useAppStore(980-2800)libs/types/src/settings.ts (1)
MCPServerConfig(186-211)apps/ui/src/hooks/use-settings-migration.ts (2)
loadMCPServersFromServer(335-361)syncSettingsToServer(196-247)
🔇 Additional comments (10)
apps/ui/src/lib/http-api-client.ts (1)
929-942: LGTM! Type definitions align with MCP server configuration.The extended response type correctly includes the new MCP server configuration fields and permission flags, matching the types defined in
libs/types/src/settings.ts.apps/ui/src/hooks/use-settings-migration.ts (2)
231-233: MCP fields correctly added to settings sync.The sync payload now includes the three new MCP-related fields, ensuring they're persisted to the server settings file alongside other global settings.
335-361:loadMCPServersFromServerimplementation is correct.The function appropriately:
- Guards for Electron-only execution
- Validates the server response
- Applies defaults for missing permission flags
- Uses
useAppStore.setState(correct for non-hook context)- Logs outcomes for debugging
libs/types/src/settings.ts (1)
166-211: Well-structured MCP configuration types.The
MCPToolInfoandMCPServerConfiginterfaces provide comprehensive coverage of the three transport types (stdio, SSE, HTTP) with appropriate optional fields for each. Documentation clearly explains each field's purpose.apps/ui/src/components/views/settings-view.tsx (1)
21-21: MCP Servers section correctly integrated.The new section follows the established pattern: import at the top and render based on
activeView. Consistent with other settings sections.Also applies to: 120-121
apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts (1)
6-6: Type extension correctly adds MCP servers view.The new
'mcp-servers'option integrates cleanly into the existing settings navigation system.apps/server/src/lib/settings-helpers.ts (3)
149-183:getMCPServersFromSettingscorrectly loads and converts MCP server configurations.The function:
- Safely handles missing settings service
- Filters to enabled servers only
- Converts each server to SDK format
- Provides helpful logging
- Returns empty object on error (safe fallback)
192-217:getMCPPermissionSettingsprovides sensible defaults.Returns permission settings with appropriate defaults when unavailable, and logs the resolved values for debugging. The default values match those in
DEFAULT_GLOBAL_SETTINGS.
223-256: Validation added for required fields per server type.The
convertToSdkFormatfunction now includes runtime validation (addressing the previous review), throwing descriptive errors when required fields are missing for each transport type:
- SSE/HTTP servers: validates
urlis present- Stdio servers: validates
commandis presentThis prevents runtime errors from invalid configurations and provides clear error messages for troubleshooting.
apps/server/src/providers/claude-provider.ts (1)
53-76: SDK options construction correctly applies MCP settings.The conditional logic properly:
- Restricts tools based on
shouldRestrictToolsflag- Sets
permissionModeto'bypassPermissions'when auto-approve is enabled- Includes required
allowDangerouslySkipPermissionsflag for bypass mode- Forwards
mcpServersconfiguration to the SDKThe implementation correctly handles the various combinations of MCP settings.
Add Model Context Protocol (MCP) server integration to extend AI agent capabilities with external tools. This allows users to configure MCP servers (stdio, SSE, HTTP) in global settings and have agents use them. Note: MCP servers are currently configured globally. Per-project MCP server configuration is planned for a future update. Features: - New MCP Servers settings section with full CRUD operations - Import/Export JSON configs (Claude Code format compatible) - Configurable permission settings: - Auto-approve MCP tools (bypass permission prompts) - Unrestricted tools (allow all tools when MCP enabled) - Refresh button to reload from settings file Implementation: - Added MCPServerConfig and MCPToolInfo types - Added store actions for MCP server management - Updated claude-provider to use configurable MCP permissions - Updated sdk-options factory functions for MCP support - Added settings helpers for loading MCP configs
mzubair481
force-pushed
the
feature/mcp-server-support
branch
from
6e33de9 to
5f328a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/types/src/provider.ts (1)
42-47: Design tradeoff acknowledged: optionaltypefor stdio breaks discriminated union narrowing.The comment explains this matches SDK behavior for simpler configs, but be aware that TypeScript's discriminated union narrowing won't work with this union:
// This won't narrow correctly: if (config.type === 'stdio') { /* ... */ } // Because config.type could be undefined for stdio configsIf type narrowing is needed in consuming code, you'll need runtime checks like:
if (config.type === 'stdio' || (!config.type && 'command' in config))This is acceptable if the SDK compatibility benefit outweighs the typing inconvenience.
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
151-152: Known limitation: argument splitting doesn't handle quoted paths.The current
.split(/\s+/)approach will incorrectly split arguments containing spaces (e.g.,"/path/with spaces" --flag).For a settings UI, this may be acceptable with clear documentation in the placeholder. However, if users need to specify paths with spaces, consider either:
- Accepting JSON array format in the input
- Using a more sophisticated shell-like argument parser
This is a minor UX concern since most MCP server arguments don't contain spaces.
🧹 Nitpick comments (1)
apps/server/src/providers/claude-provider.ts (1)
39-51: Consider extracting shared MCP permission logic to a utility function.The MCP permission logic here mirrors
buildMcpOptions()insdk-options.ts. While the comment acknowledges this intentional duplication, extracting shared logic into a utility would:
- Ensure consistent behavior if permission rules change
- Reduce maintenance burden
- Make testing easier
The logic itself is correct - this is purely a maintainability suggestion.
🔎 Suggested approach
Create a shared helper in
sdk-options.tsor a newmcp-utils.ts:export function computeMcpPermissions(options: { mcpServers?: Record<string, unknown>; mcpAutoApproveTools?: boolean; mcpUnrestrictedTools?: boolean; }) { const hasMcpServers = options.mcpServers && Object.keys(options.mcpServers).length > 0; const mcpAutoApprove = options.mcpAutoApproveTools ?? true; const mcpUnrestricted = options.mcpUnrestrictedTools ?? true; return { shouldBypassPermissions: hasMcpServers && mcpAutoApprove, shouldRestrictTools: !hasMcpServers || !mcpUnrestricted, }; }Then use it in both
claude-provider.tsandbuildMcpOptions().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/server/package.jsonapps/server/src/lib/sdk-options.tsapps/server/src/lib/settings-helpers.tsapps/server/src/providers/claude-provider.tsapps/server/src/providers/types.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/components/views/context-view.tsxapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/config/navigation.tsapps/ui/src/components/views/settings-view/hooks/use-settings-view.tsapps/ui/src/components/views/settings-view/mcp-servers/index.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/store/app-store.tslibs/types/src/index.tslibs/types/src/provider.tslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/ui/src/components/views/settings-view/mcp-servers/index.ts
- apps/server/package.json
- apps/ui/src/components/views/context-view.tsx
- apps/ui/src/components/views/settings-view/hooks/use-settings-view.ts
- apps/server/src/services/agent-service.ts
- apps/ui/src/lib/http-api-client.ts
- apps/ui/src/components/views/settings-view.tsx
- apps/ui/src/components/views/settings-view/config/navigation.ts
- libs/types/src/settings.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/server/src/services/auto-mode-service.ts (2)
apps/server/src/lib/settings-helpers.ts (2)
getMCPServersFromSettings(149-183)getMCPPermissionSettings(192-217)apps/server/src/lib/sdk-options.ts (1)
createAutoModeOptions(431-460)
apps/ui/src/store/app-store.ts (1)
libs/types/src/settings.ts (1)
MCPServerConfig(186-211)
apps/ui/src/hooks/use-settings-migration.ts (2)
apps/ui/src/lib/electron.ts (1)
isElectron(655-665)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(1147-1152)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (4)
apps/ui/src/store/app-store.ts (1)
useAppStore(980-2800)libs/types/src/index.ts (1)
MCPServerConfig(62-62)libs/types/src/settings.ts (1)
MCPServerConfig(186-211)apps/ui/src/hooks/use-settings-migration.ts (2)
loadMCPServersFromServer(335-361)syncSettingsToServer(196-247)
🔇 Additional comments (28)
libs/types/src/index.ts (1)
16-19: LGTM! Type exports properly expose the new MCP configuration surface.The dual naming convention (
McpServerConfigfor SDK format vsMCPServerConfigfor settings format) is intentional as they serve different purposes - SDK-level transport configuration vs application-level persistence with metadata.Also applies to: 61-62
apps/server/src/providers/claude-provider.ts (1)
58-75: LGTM! SDK options construction correctly applies MCP settings.The conditional spreading pattern is clean and correctly:
- Applies tool restrictions only when needed
- Sets
permissionModebased on bypass conditions- Includes
allowDangerouslySkipPermissionsonly when bypassing- Forwards MCP servers when configured
apps/server/src/services/auto-mode-service.ts (5)
1846-1862: LGTM! MCP settings correctly loaded and applied to SDK options.MCP servers and permissions are now properly loaded at the start of
runAgentand propagated to bothcreateAutoModeOptionsandexecuteOptions.
1893-1906: LGTM! MCP settings correctly forwarded in initial executeOptions.The initial agent execution now properly includes all MCP-related fields.
2126-2136: Previous issue addressed: MCP settings now included in revision stream.The plan revision
executeQuerycall now correctly includesmcpServers,mcpAutoApproveTools, andmcpUnrestrictedTools, ensuring consistent MCP behavior across all execution phases.
2263-2276: Previous issue addressed: MCP settings now included in task execution.Per-task
executeQuerycalls now properly propagate MCP configuration, maintaining consistent tool access throughout multi-agent task execution.
2352-2368: Previous issue addressed: MCP settings now included in single-agent fallback.The continuation/fallback implementation path now correctly includes MCP settings, completing the fix for consistent MCP propagation across all execution paths.
libs/types/src/provider.ts (1)
73-75: LGTM! ExecuteOptions properly extended with MCP fields.The new optional fields correctly integrate MCP configuration into the provider execution interface with clear inline documentation.
apps/ui/src/hooks/use-settings-migration.ts (2)
231-233: LGTM! MCP settings properly included in server sync.The
syncSettingsToServerfunction now correctly persists MCP-related state (mcpServers,mcpAutoApproveTools,mcpUnrestrictedTools) to the settings file.
335-361: LGTM!loadMCPServersFromServerimplementation is sound.The function correctly:
- Guards for Electron-only execution
- Validates the API response before accessing properties
- Updates the store with sensible defaults for missing flags (
true)- Uses
useAppStore.setStateappropriately for non-hook context- Returns boolean success indicator with proper error handling
apps/server/src/lib/settings-helpers.ts (3)
149-183: LGTM! MCP server loading with proper filtering and conversion.The function correctly:
- Handles missing settings service gracefully
- Filters to only enabled servers
- Converts each server to SDK format via validated helper
- Logs loaded server names for debugging
- Returns empty object on error rather than throwing (resilient design)
192-217: LGTM! MCP permission settings with sensible defaults.The function provides consistent defaults (
truefor both flags) when settings are unavailable, matching the intentional "permissive by default" design choice documented elsewhere.
223-256: Previous review concern addressed:convertToSdkFormatnow validates required fields.The implementation throws descriptive errors when required fields are missing (e.g.,
urlfor SSE/HTTP,commandfor stdio), preventing runtime errors from invalid configurations. This is a significant improvement over the previous non-null assertion approach.apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (4)
78-83: LGTM! Auto-load on mount with proper error handling.The
useEffectcorrectly loads MCP servers from the settings file when the component mounts, with error logging for debugging.
126-168: LGTM! Form validation and save logic is comprehensive.The validation correctly enforces:
- Name is always required
- Command required for stdio type
- URL required for SSE/HTTP types
The server data construction properly handles both create and update flows.
372-400: Previous issue addressed: Permission toggles now sync to server.Both
mcpAutoApproveToolsandmcpUnrestrictedToolsswitches now correctly callsyncSettingsToServer()after updating state, ensuring changes persist to the settings file.
188-270: LGTM! Import/export implementation is robust.The import handler:
- Supports both Claude Code format and direct format
- Deduplicates by name
- Validates required fields per server type
- Provides clear feedback via toasts
The export handler correctly serializes to Claude Code-compatible JSON format.
apps/server/src/providers/types.ts (1)
7-16: LGTM! Type re-exports consolidate MCP types from shared package.Re-exporting from
@automaker/typesensures type consistency across the codebase and eliminates potential drift between duplicate type definitions. The comment clearly documents the re-export pattern.apps/ui/src/store/app-store.ts (5)
486-489: LGTM! MCP state fields properly typed and documented.The new MCP configuration fields are well-integrated into the app state with appropriate types and clear documentation.
1590-1601: LGTM! MCP permission setters follow established patterns.Both actions correctly update state and sync to the server settings file, maintaining consistency with other SDK-related settings like
setAutoLoadClaudeMdandsetEnableSandboxMode.
1643-1664: LGTM! MCP server CRUD operations properly implemented.The MCP server management actions follow existing store patterns:
- ID generation is consistent with other entities
- Immutability is preserved using spread, map, and filter
- New servers default to
enabled: true, which is sensible UX
949-951: Verify that permissive MCP defaults are intentional.The defaults for MCP permission settings are both
true:
mcpAutoApproveTools: truebypasses permission prompts for MCP tool callsmcpUnrestrictedTools: trueremoves tool restrictions when MCP is enabledWhile the comments clearly state these are deliberate choices for "ease of use," these permissive defaults may have security implications, especially if users aren't aware they need to explicitly opt into stricter security.
Please confirm these defaults align with the project's security model and that users will be adequately informed about what these settings control.
Related: Similar defaults exist in
apps/server/src/lib/sdk-options.tslines 163-164, where past reviewers raised security concerns about bypassing permission checks by default.
2771-2774: LGTM! MCP settings properly persisted.The MCP configuration is correctly included in the partialize function, ensuring settings survive app restarts alongside other user preferences.
apps/server/src/lib/sdk-options.ts (5)
270-277: LGTM! MCP configuration fields properly typed and documented.The extension of
CreateSdkOptionsConfigwith MCP-related fields is well-structured with clear comments explaining each setting's purpose.
280-286: LGTM! Convenient re-exports for MCP types.Re-exporting MCP configuration types from this module provides a convenient API surface for consumers of the SDK options factory.
397-417: LGTM! MCP options properly integrated in chat mode.The
createChatOptionsfunction correctly leveragesbuildMcpOptionsto:
- Conditionally restrict tools based on MCP configuration
- Apply permission bypass settings when appropriate
- Include MCP server configurations
The integration is clean and follows the helper's API correctly.
438-458: LGTM! MCP options properly integrated in auto mode.The
createAutoModeOptionsfunction correctly usesbuildMcpOptionswith the same pattern ascreateChatOptions, appropriately using thefullAccesstool preset for autonomous feature implementation.
481-502: LGTM! MCP integration in custom options addresses past feedback.The
createCustomOptionsfunction now consistently applies MCP logic, addressing the inconsistency flagged in previous reviews. The fallback logic foreffectiveAllowedToolsis well-designed:
- Respects explicit
allowedToolswhen provided- Falls back to restricted preset when MCP requires restrictions
- Allows unrestricted tools when MCP unrestricted mode is enabled
This provides flexibility while maintaining consistency with other option builders.
mzubair481
changed the title
|
@mzubair481 Could u please address the gemini / code rabbit review comments before we do the full code review |
|
Hi @Shironex , I've gone through all the Gemini and CodeRabbit comments. Here's a summary: ✅ Fixed (9 items)
⚪ Design Decisions (intentionally kept as-is)
|
- Add tests for getMCPServersFromSettings() - Add tests for getMCPPermissionSettings() - Cover all server types (stdio, sse, http) - Test error handling and edge cases - Increases branch coverage from 54.91% to 56.59%
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/server/tests/unit/lib/settings-helpers.test.ts (2)
193-211: Update test names to reflect actual behavior.The test names say "should throw error" but the function catches errors and returns an empty object. The assertions correctly expect
{}, but the naming is misleading.🔎 Suggested test name updates
- it('should throw error for SSE server without URL', async () => { + it('should return empty object for SSE server without URL', async () => {- it('should throw error for HTTP server without URL', async () => { + it('should return empty object for HTTP server without URL', async () => {- it('should throw error for stdio server without command', async () => { + it('should return empty object for stdio server without command', async () => {Also applies to: 213-230, 232-249
251-273: Consider testing behavior rather than implementation details.The expectation includes
args: undefined, env: undefined, which couples the test to implementation details. If the function later omits undefined fields, this test would break despite correct behavior.🔎 More resilient assertion
const result = await getMCPServersFromSettings(mockSettingsService); - expect(result['no-type']).toEqual({ - type: 'stdio', - command: 'node', - args: undefined, - env: undefined, - }); + expect(result['no-type']).toMatchObject({ + type: 'stdio', + command: 'node', + });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/tests/unit/lib/settings-helpers.test.ts
🔇 Additional comments (2)
apps/server/tests/unit/lib/settings-helpers.test.ts (2)
1-10: LGTM!The imports and test setup are well-structured. Mocking console methods in beforeEach prevents test output noise and allows verification of logging behavior.
276-364: LGTM!The
getMCPPermissionSettingstest suite is comprehensive and well-structured. It covers null/undefined services, default behavior, mixed settings, error handling, and custom log prefixes.
|
@mzubair481 Thanks for addressing them. We’ll review it, but from a first glance, it looks really promising and should be easy to merge with maybe small tweaks/changes for #288 |
Shironex
added
Testers-Requested
- Add MCPTestService for testing MCP server connections - Support stdio, SSE, and HTTP transport types - Implement workaround for SSE headers bug (SDK Issue AutoMaker-Org#436) - Create API routes for /api/mcp/test and /api/mcp/tools - Add API client methods for MCP operations - Create MCPToolsList component with collapsible schema display - Add Test button to MCP servers section with status indicators - Add Headers field for HTTP/SSE servers - Add Environment Variables field for stdio servers - Fix text overflow in tools list display 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Introduced JSON editing for individual and global MCP server configurations. - Added functionality to open JSON edit dialogs for specific servers and all servers collectively. - Implemented validation for JSON input to ensure correct server configuration. - Enhanced server testing logic to allow silent testing without toast notifications. - Updated UI to include buttons for editing JSON configurations and improved user experience. This update streamlines server management and configuration, allowing for more flexible and user-friendly interactions.
|
@mzubair481 I added a few things:
The testing feature is useful because I had trouble connecting one of my MCPs and this gives a clear view if the integration is working as expected. Also makes it easier to copy MCP configs from other CLIs/IDEs. Changes
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/types/src/settings.ts (1)
522-524: Acknowledged: Permissive MCP defaults retained per design decision.Per the PR objectives, the permissive defaults (
mcpAutoApproveTools: true,mcpUnrestrictedTools: true) are intentional for ease of use and parity with Claude Code CLI. This was explicitly called out as a design decision in the PR discussion.
🧹 Nitpick comments (6)
apps/ui/src/lib/http-api-client.ts (1)
929-942: Consider importing MCPServerConfig from @automaker/types.The MCP server configuration type is duplicated inline here rather than imported from
@automaker/types. This creates maintenance overhead if the type changes.🔎 Suggested improvement
+import type { MCPServerConfig, MCPToolInfo } from '@automaker/types'; + // Then use in the response type: - mcpServers?: Array<{ - id: string; - name: string; - description?: string; - type?: 'stdio' | 'sse' | 'http'; - command?: string; - args?: string[]; - env?: Record<string, string>; - url?: string; - headers?: Record<string, string>; - enabled?: boolean; - }>; + mcpServers?: MCPServerConfig[];apps/server/src/routes/mcp/common.ts (1)
8-13: Code duplication:getErrorMessageexists in MCPTestService.This utility function is duplicated in
apps/server/src/services/mcp-test-service.ts(lines 201-206) as a private method. Consider having MCPTestService import from this common module to avoid duplication.🔎 Suggested change in mcp-test-service.ts
+import { getErrorMessage } from '../routes/mcp/common.js'; // Then remove the private getErrorMessage method from MCPTestService - private getErrorMessage(error: unknown): string { - if (error instanceof Error) { - return error.message; - } - return String(error); - }apps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsx (1)
7-12: Consider reusingMCPToolInfofrom@automaker/types.
MCPToolDisplayis nearly identical toMCPToolInfodefined inlibs/types/src/settings.ts. Consider importing and extending the shared type to reduce duplication.🔎 Suggested approach
+import type { MCPToolInfo } from '@automaker/types'; + -export interface MCPToolDisplay { - name: string; - description?: string; - inputSchema?: Record<string, unknown>; - enabled: boolean; -} +// Reuse the shared type +export type MCPToolDisplay = MCPToolInfo;apps/server/src/services/mcp-test-service.ts (3)
84-92: Consider extracting server version from capabilities.The MCP client receives server info during the connection handshake. You could extract the actual server version instead of hardcoding
undefined.🔎 Proposed enhancement
return { success: true, tools, connectionTime, serverInfo: { name: serverConfig.name, - version: undefined, // Could be extracted from server info if available + version: client.getServerVersion?.()?.toString(), }, };Note: Verify the exact API for retrieving server version from the MCP SDK client.
28-36: Per coding guidelines, server operations should emit events viacreateEventEmitter(), but MCPTestService is intentionally request/response.MCPTestService completes within 10 seconds and returns a single result object, making it unsuitable for WebSocket streaming. If you want to add progress feedback ("connecting...", "listing tools..."), the service would need to accept an
EventEmitterinstance and emit events during the connection and tool-listing phases. This is an architectural choice—the current approach is acceptable for a diagnostic endpoint, but refactoring to follow the guideline would improve consistency with other server operations.
183-188: Mergeprocess.envwith custom environment variables to prevent child process startup failures.The MCP SDK's
StdioClientTransportonly whitelists specific environment variables (PATH, HOME, USER, SHELL, etc.) when noenvis provided. Ifconfig.envis passed as-is, it completely replaces the environment, potentially causing the spawned process to fail if it needs PATH or other system variables. This pattern is already used elsewhere in the codebase (e.g.,terminal-service.ts).return new StdioClientTransport({ command: config.command, args: config.args, - env: config.env, + env: config.env ? { ...process.env, ...config.env } : undefined, });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/server/src/index.tsapps/server/src/routes/mcp/common.tsapps/server/src/routes/mcp/index.tsapps/server/src/routes/mcp/routes/list-tools.tsapps/server/src/routes/mcp/routes/test-server.tsapps/server/src/services/mcp-test-service.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsxapps/ui/src/lib/http-api-client.tslibs/types/src/settings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/server/src/routes/mcp/index.tsapps/server/src/routes/mcp/routes/list-tools.tsapps/server/src/routes/mcp/routes/test-server.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsxapps/server/src/services/mcp-test-service.tsapps/ui/src/lib/http-api-client.tslibs/types/src/settings.tsapps/server/src/index.tsapps/server/src/routes/mcp/common.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/mcp/index.tsapps/server/src/routes/mcp/routes/list-tools.tsapps/server/src/routes/mcp/routes/test-server.tsapps/server/src/services/mcp-test-service.tsapps/server/src/index.tsapps/server/src/routes/mcp/common.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/server/src/routes/mcp/index.tsapps/server/src/routes/mcp/routes/list-tools.tsapps/server/src/routes/mcp/routes/test-server.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsxapps/server/src/services/mcp-test-service.tsapps/ui/src/lib/http-api-client.tslibs/types/src/settings.tsapps/server/src/index.tsapps/server/src/routes/mcp/common.ts
🧬 Code graph analysis (6)
apps/server/src/routes/mcp/index.ts (3)
apps/server/src/services/mcp-test-service.ts (1)
MCPTestService(31-208)apps/server/src/routes/mcp/routes/test-server.ts (1)
createTestServerHandler(27-62)apps/server/src/routes/mcp/routes/list-tools.ts (1)
createListToolsHandler(27-67)
apps/server/src/routes/mcp/routes/list-tools.ts (3)
libs/types/src/settings.ts (1)
MCPServerConfig(188-213)apps/server/src/services/mcp-test-service.ts (2)
MCPTestService(31-208)getErrorMessage(202-207)apps/server/src/routes/mcp/common.ts (2)
logError(18-20)getErrorMessage(8-13)
apps/server/src/routes/mcp/routes/test-server.ts (3)
libs/types/src/settings.ts (1)
MCPServerConfig(188-213)apps/server/src/services/mcp-test-service.ts (2)
MCPTestService(31-208)getErrorMessage(202-207)apps/server/src/routes/mcp/common.ts (2)
logError(18-20)getErrorMessage(8-13)
libs/types/src/settings.ts (1)
libs/types/src/index.ts (2)
MCPToolInfo(61-61)MCPServerConfig(62-62)
apps/server/src/index.ts (2)
apps/server/src/services/mcp-test-service.ts (1)
MCPTestService(31-208)apps/server/src/routes/mcp/index.ts (1)
createMCPRoutes(26-36)
apps/server/src/routes/mcp/common.ts (1)
apps/server/src/services/mcp-test-service.ts (1)
getErrorMessage(202-207)
🔇 Additional comments (18)
libs/types/src/settings.ts (2)
166-213: Well-structured MCP type definitions.The
MCPToolInfoandMCPServerConfiginterfaces are comprehensive and well-documented, covering all three transport types (stdio, SSE, HTTP) with appropriate optional fields. The JSDoc comments clearly explain each property's purpose.
355-362: MCP settings integration looks good.The new MCP properties are cleanly integrated into
GlobalSettingswith appropriate optional types for the permission flags. The inline comments clearly document the security implications of each setting.apps/ui/src/lib/http-api-client.ts (1)
1142-1202: MCP API namespace implementation looks good.The three MCP methods (
testServer,testServerConfig,listTools) are well-structured with consistent response types. The separation between testing by ID vs. testing with a full config provides good flexibility.apps/server/src/routes/mcp/common.ts (1)
15-19: LGTM - Simple logging utility.The
logErrorfunction provides consistent error logging with an[MCP]prefix for easy filtering in logs.apps/server/src/routes/mcp/index.ts (1)
26-35: Clean router factory implementation.The
createMCPRoutesfactory follows the established pattern in the codebase for dependency injection of services. The JSDoc documentation clearly lists available endpoints.apps/server/src/index.ts (3)
53-54: MCP imports properly placed.Imports follow the existing organization pattern in the file.
124-124: MCPTestService instantiation with proper dependency.The service is correctly instantiated with
settingsServiceas a dependency, following the established pattern for other services in this file.
168-168: MCP routes protected by authentication.The MCP routes are mounted after the
authMiddlewareis applied (line 145), ensuring they're protected like other API endpoints.apps/server/src/routes/mcp/routes/list-tools.ts (1)
27-66: Handler structure follows established patterns.The factory function, error handling, and response structure are consistent with other route handlers in the codebase.
apps/server/src/routes/mcp/routes/test-server.ts (1)
27-61: Handler implementation is clean and consistent.The test server handler follows the same pattern as list-tools, with proper error handling and response formatting.
apps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsx (2)
63-63: Potential key collision if tool names are not unique.Using
tool.nameas the React key assumes tool names are unique within a server. If an MCP server could return tools with duplicate names (unlikely but possible), this could cause rendering issues.Consider whether tool names are guaranteed to be unique per server, or if a more robust key like
${tool.name}-${index}would be safer.
21-116: Well-implemented collapsible tools list.The component handles loading, error, and empty states appropriately. The collapsible schema display with proper overflow handling provides good UX for exploring tool definitions.
apps/server/src/services/mcp-test-service.ts (6)
8-15: LGTM!Imports correctly use
@automaker/typesfor shared types per coding guidelines. The default timeout of 10 seconds is reasonable for MCP connection testing.
17-26: LGTM!The
MCPTestResultinterface is well-structured with appropriate optional fields for success and failure scenarios.
100-109: LGTM!Good use of the
finallyblock for cleanup. Ignoring cleanup errors is appropriate here to avoid masking actual test results.
112-134: LGTM!Clean implementation with proper null-safe access to
mcpServersand clear error messaging when the server is not found.
142-162: Well-documented SDK workaround.Good job documenting the SSE headers workaround with a link to the upstream issue. This makes the non-obvious code much easier to maintain.
190-207: LGTM!The helper methods are cleanly implemented. The
timeoutfunction works well withPromise.race, andgetErrorMessageproperly handles unknown error types.
Resolved conflicts: - apps/server/src/index.ts: merged MCP and Pipeline routes - apps/ui/src/lib/http-api-client.ts: merged MCP and Pipeline APIs - apps/ui/src/store/app-store.ts: merged type imports 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
CodeRabbit identified dead code - the else blocks were unreachable since validation ensures serverId or serverConfig is truthy. Simplified to ternary expression. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/ui/src/lib/http-api-client.ts (2)
929-942: Consider importing shared MCP types from@automaker/types.The MCP server config shape is defined inline here, but
MCPServerConfigis already exported from@automaker/types. Importing the shared type would reduce duplication and ensure consistency across the codebase.As per coding guidelines, prefer imports from shared packages (
@automaker/*).🔎 Proposed refactor
Add import at the top of the file:
import type { MCPServerConfig } from '@automaker/types';Then reference the type in the settings response:
lastSelectedSessionByProject: Record<string, string>; - mcpServers?: Array<{ - id: string; - name: string; - description?: string; - type?: 'stdio' | 'sse' | 'http'; - command?: string; - args?: string[]; - env?: Record<string, string>; - url?: string; - headers?: Record<string, string>; - enabled?: boolean; - }>; + mcpServers?: MCPServerConfig[]; mcpAutoApproveTools?: boolean; mcpUnrestrictedTools?: boolean;
1143-1202: MCP API methods look correct; consider reusing shared types.The API structure is well-designed with clear separation between
testServer(by ID) andtestServerConfig(by config object). TheserverConfigparameter type (lines 1163-1173) duplicatesMCPServerConfig, and the tool item shape could useMCPToolInfofrom@automaker/types.🔎 Optional: Use shared types for consistency
import type { MCPServerConfig, MCPToolInfo } from '@automaker/types'; // Then in testServerConfig: testServerConfig: (serverConfig: MCPServerConfig): Promise<{ success: boolean; tools?: MCPToolInfo[]; // ... }> => this.post('/api/mcp/test', { serverConfig }),apps/server/src/services/auto-mode-service.ts (1)
2001-2060: MCP configuration correctly propagated to all executeQuery callsThe MCP servers and permission settings are now consistently passed to all agent execution phases (initial, revision, task, continuation), which addresses the concern from the previous review. The implementation is functionally correct.
Optional: Reduce code duplication by extracting MCP options
The MCP configuration object is constructed identically in four places. Consider extracting it once after loading the settings:
const mcpServers = await getMCPServersFromSettings(this.settingsService, '[AutoMode]'); const mcpPermissions = await getMCPPermissionSettings(this.settingsService, '[AutoMode]'); + +// Shared MCP options for all executeQuery calls +const mcpExecuteOptions = { + mcpServers: Object.keys(mcpServers).length > 0 ? mcpServers : undefined, + mcpAutoApproveTools: mcpPermissions.mcpAutoApproveTools, + mcpUnrestrictedTools: mcpPermissions.mcpUnrestrictedTools, +};Then spread it into all executeQuery calls:
const executeOptions: ExecuteOptions = { prompt: promptContent, model: finalModel, maxTurns: maxTurns, cwd: workDir, allowedTools: allowedTools, abortController, systemPrompt: sdkOptions.systemPrompt, settingSources: sdkOptions.settingSources, sandbox: sdkOptions.sandbox, - mcpServers: Object.keys(mcpServers).length > 0 ? mcpServers : undefined, - mcpAutoApproveTools: mcpPermissions.mcpAutoApproveTools, - mcpUnrestrictedTools: mcpPermissions.mcpUnrestrictedTools, + ...mcpExecuteOptions, };Apply the same pattern to the revision stream (line 2281), task stream (line 2421), and continuation stream (line 2513).
This matches the refactoring approach suggested in the earlier review and improves maintainability.
Also applies to: 2288-2290, 2428-2430, 2520-2522
apps/ui/src/store/app-store.ts (1)
1672-1675: Consider respecting explicitenabledinput values.The current implementation always sets
enabled: true, overriding any value passed in the input. While the PR objectives indicate this is intentional ("Creation assigns...enabled flag"), it creates an API inconsistency:
- The input type
Omit<MCPServerConfig, 'id'>allowsenabledto be passed- Other similar functions (e.g.,
addAIProfile) don't override input fields🔎 Suggested approach to default while respecting input
addMCPServer: (server) => { const id = `mcp-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; - set({ mcpServers: [...get().mcpServers, { ...server, id, enabled: true }] }); + set({ mcpServers: [...get().mcpServers, { enabled: true, ...server, id }] }); },This defaults
enabledtotruebut respects an explicitfalsevalue if provided, making the API more predictable while maintaining the desired default behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/server/src/index.tsapps/server/src/routes/mcp/routes/list-tools.tsapps/server/src/routes/mcp/routes/test-server.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/store/app-store.tslibs/types/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/routes/mcp/routes/test-server.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
libs/types/src/index.tsapps/server/src/routes/mcp/routes/list-tools.tsapps/server/src/index.tsapps/ui/src/lib/http-api-client.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/store/app-store.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
libs/types/src/index.tsapps/server/src/routes/mcp/routes/list-tools.tsapps/server/src/index.tsapps/ui/src/lib/http-api-client.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/store/app-store.ts
apps/server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
createEventEmitter()fromlib/events.tsfor all server operations to emit events that stream to frontend via WebSocket
Files:
apps/server/src/routes/mcp/routes/list-tools.tsapps/server/src/index.tsapps/server/src/services/auto-mode-service.ts
🧬 Code graph analysis (3)
apps/server/src/index.ts (2)
apps/server/src/services/mcp-test-service.ts (1)
MCPTestService(31-208)apps/server/src/routes/mcp/index.ts (1)
createMCPRoutes(26-36)
apps/server/src/services/auto-mode-service.ts (2)
apps/server/src/lib/settings-helpers.ts (2)
getMCPServersFromSettings(149-183)getMCPPermissionSettings(192-217)apps/server/src/lib/sdk-options.ts (1)
createAutoModeOptions(431-460)
apps/ui/src/store/app-store.ts (2)
libs/types/src/settings.ts (1)
MCPServerConfig(188-213)apps/ui/src/hooks/use-settings-migration.ts (1)
syncSettingsToServer(196-247)
🔇 Additional comments (16)
libs/types/src/index.ts (2)
16-19: LGTM!The MCP provider type exports follow the established barrel export pattern. The casing distinction (
Mcp*for SDK types) is intentional per the documented design decision.
61-62: LGTM!The settings-related MCP type exports (
MCPToolInfo,MCPServerConfig) are properly added. TheMCP*casing for UI/settings types maintains the intentional distinction from SDK types.apps/server/src/index.ts (3)
53-54: LGTM! Clean MCP imports.The imports follow the existing ESM pattern with proper
.jsextensions and match the expected signatures from the MCP route and service modules.
126-126: LGTM! Proper service instantiation.The
MCPTestServiceis correctly instantiated with the requiredsettingsServicedependency, following the same pattern as other services in this file.
170-170: LGTM! Correct MCP route mounting.The MCP routes are properly mounted at
/api/mcpwith the requiredmcpTestServicedependency. The routes are correctly positioned after the authentication middleware (line 147), ensuring they are protected.apps/server/src/routes/mcp/routes/list-tools.ts (3)
1-22: LGTM! Well-documented endpoint with clear types.The JSDoc comment clearly documents the endpoint's purpose and contract, and the
ListToolsRequestinterface appropriately models the either/or requirement forserverIdandserverConfig.
27-50: LGTM! Clean handler implementation.The validation logic correctly ensures either
serverIdorserverConfigis provided. The ternary operator (lines 41-43) cleanly handles both cases, and the non-null assertion onbody.serverConfig!is safe because the validation guarantees at least one value is truthy. The previous unreachable code issue has been properly addressed.
51-58: LGTM! Appropriate error handling.The error handling uses the shared
logErrorandgetErrorMessageutilities, provides proper context for debugging, and returns an appropriate 500 response without leaking sensitive information.apps/server/src/services/auto-mode-service.ts (1)
40-41: LGTM: MCP settings helpers imported correctlyThe new imports for MCP server and permission settings are correctly added and used consistently throughout the file.
apps/ui/src/store/app-store.ts (7)
10-10: LGTM: Clean import from shared package.The MCPServerConfig type is correctly imported from
@automaker/types, following the coding guideline to use shared packages.
490-493: LGTM: Well-structured MCP state fields.The MCP state fields are properly typed and documented. The field names clearly convey their purpose.
774-775: LGTM: Consistent action signatures.The MCP action signatures follow established patterns in the codebase:
- Permission setters are async (for syncing to server)
- CRUD operations match the patterns used for AI profiles and projects
Also applies to: 784-788
972-974: LGTM: Sensible defaults with clear documentation.The initial MCP state is well thought out:
- Empty server list is appropriate for new installations
- Permissive defaults (auto-approve and unrestricted tools enabled) align with the stated design goal of ease of use
1618-1629: LGTM: Proper server synchronization.Both permission setters correctly sync to the server settings file using the established pattern. The dynamic import and await ensure proper sequencing without circular dependencies.
1677-1692: LGTM: Standard CRUD operations.The update, remove, and reorder operations follow established patterns used elsewhere in the store (projects, AI profiles). The implementation is clean and consistent.
2908-2911: LGTM: Complete MCP state persistence.All MCP-related state fields are correctly included in the persisted state, ensuring MCP configuration survives page reloads. The comment clearly identifies the section.
Only auto-expand servers when user manually clicks Test button. Auto-test on mount now keeps servers collapsed to avoid clutter when there are many MCP servers configured. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
312-312: Known limitation: Argument parsing doesn't handle quoted strings.The current
.split(/\s+/)implementation doesn't handle arguments with spaces (e.g.,"/path/with spaces"). This is noted as an intentional design decision by the author, with quoted-argument parsing deferred to future work.
🧹 Nitpick comments (1)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
80-1268: Consider splitting into smaller components.While the current implementation is functional and well-organized, the component handles many responsibilities (~1268 lines). Consider extracting:
- Server list item rendering logic
- Individual dialog components (add/edit, import, export, JSON editing)
- Permission settings section
This would improve maintainability and testability. However, this is optional—the current structure is cohesive and acceptable for a settings UI.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
🧬 Code graph analysis (1)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (6)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsx (2)
MCPToolDisplay(7-12)MCPToolsList(21-117)apps/ui/src/store/app-store.ts (1)
useAppStore(1008-2937)libs/types/src/settings.ts (1)
MCPServerConfig(188-213)apps/ui/src/hooks/use-settings-migration.ts (2)
loadMCPServersFromServer(335-361)syncSettingsToServer(196-247)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)apps/ui/src/components/ui/collapsible.tsx (3)
Collapsible(10-10)CollapsibleTrigger(10-10)CollapsibleContent(10-10)
🔇 Additional comments (8)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (8)
1-47: LGTM! Imports follow best practices.All imports are correct and follow the coding guidelines. The component imports from the shared
@automaker/typespackage and uses React 19-compatible APIs.
107-111: LGTM! Auto-load pattern is correct.The auto-load effect correctly fetches MCP servers on mount with appropriate error handling. Silent error handling is acceptable here since users can manually refresh if needed.
114-166: LGTM! Test server logic is well-implemented.The
testServercallback correctly handles both manual and automatic testing scenarios. Thesilentparameter elegantly prevents UI clutter during auto-test while maintaining user feedback for manual tests.
169-182: LGTM! Auto-test logic is sound.The auto-test effect correctly tests all enabled servers on mount while avoiding redundant tests. Parallel testing is appropriate for a settings UI where the number of servers is typically small.
776-803: LGTM! Permission toggles now persist correctly.Both permission switches correctly call
syncSettingsToServer()after updating state, ensuring changes are persisted to the settings file. The previous issue has been resolved.
354-436: LGTM! Import logic is comprehensive and well-validated.The import function correctly handles multiple JSON formats, validates configurations, checks for duplicates, and provides clear feedback to users about the import results.
464-562: LGTM! JSON editing logic is robust.The individual server JSON editing correctly handles validation, type-specific field management, and ensures unused fields are cleared when switching transport types.
564-689: LGTM! Global JSON editing is well-implemented.The global editing logic correctly handles the complex scenario of updating, adding, and removing multiple servers. Validation occurs before any changes are applied, preventing partial updates on error.
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
Outdated
Show resolved
Hide resolved
I think you have leaked your API key if that's not the case then you're good to go for future I recommend adding a masking option to the commands so if users of auto maker or anybody shares the screen. They don't accidentally leak their API key and I suggest adding a warning if user starts exceeding more than 80 mcp tools which degrades the performance of the models Note: it is much better if the MCP servers are project based and not global but this is a future enhancement |
|
@mzubair481 oh ye my bad but already changed it it was only used for testing so its fine and ye masking option to prevent situaton like this one might be good idea will implement it once done testing for now i runned test using mcp / pipelines from main branch and im already impressed with the results : ) |
|
Implemented your suggestions. For now, I’ll focus on some small refactoring and wait for Cody to review it as well. We’ll probably merge it today it seems to be working perfectly. Great work so far : ) In the meantime, I’ll look into how we should handle making it project based, as that would definitely be a nice addition. |
Security improvements: - Mask sensitive values in URLs (api_key, token, auth, secret, etc.) - Prevents accidental API key leaks when sharing screen or screenshots Performance guidance: - Show warning banner when total MCP tools exceed 80 - Warns users that high tool count may degrade AI model performance 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (2)
373-374: Argument splitting does not handle quoted paths.As noted in a previous review, the current
.split(/\s+/)implementation will incorrectly split arguments containing spaces (e.g.,"/path/with spaces"). This is a known limitation that was deferred per PR comments.
500-524: Add error handling for clipboard operation.The clipboard write operation can fail due to permission denial, browser compatibility, or other issues, but currently always shows a success toast. This was flagged in a previous review and remains unaddressed.
🔎 Proposed fix
- const handleExportJson = () => { + const handleExportJson = async () => { const exportData: Record<string, Record<string, unknown>> = {}; for (const server of mcpServers) { const serverConfig: Record<string, unknown> = { type: server.type || 'stdio', }; if (server.type === 'stdio' || !server.type) { serverConfig.command = server.command; if (server.args?.length) serverConfig.args = server.args; if (server.env && Object.keys(server.env).length > 0) serverConfig.env = server.env; } else { serverConfig.url = server.url; if (server.headers && Object.keys(server.headers).length > 0) serverConfig.headers = server.headers; } exportData[server.name] = serverConfig; } const json = JSON.stringify({ mcpServers: exportData }, null, 2); - navigator.clipboard.writeText(json); - toast.success('Copied to clipboard'); + try { + await navigator.clipboard.writeText(json); + toast.success('Copied to clipboard'); + } catch (error) { + toast.error('Failed to copy to clipboard'); + console.error('Clipboard write failed:', error); + } };
🧹 Nitpick comments (2)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (2)
231-244: Consider adding cleanup for in-flight server tests.If the component unmounts while server tests are in progress, the state updates in the callbacks may trigger React warnings. Consider using an AbortController or a mounted ref to skip state updates after unmount.
🔎 Proposed fix
// Auto-test all enabled servers on mount useEffect(() => { + let isMounted = true; const enabledServers = mcpServers.filter((s) => s.enabled !== false); const serversToTest = enabledServers.filter((s) => !autoTestedServersRef.current.has(s.id)); if (serversToTest.length > 0) { // Mark all as being tested serversToTest.forEach((s) => autoTestedServersRef.current.add(s.id)); // Test all servers in parallel (silently - no toast spam) serversToTest.forEach((server) => { - testServer(server, true); + if (isMounted) { + testServer(server, true); + } }); } + + return () => { + isMounted = false; + }; }, [mcpServers, testServer]);
736-741: Consider batching server removal operations.The loop removes servers one by one, triggering multiple store updates and potential re-renders. While functionally correct (the iteration uses a snapshot), batching these removals would be more efficient.
🔎 Proposed fix
// Remove servers that are no longer in the JSON + const serversToRemove = mcpServers.filter((server) => !processedNames.has(server.name)); - for (const server of mcpServers) { - if (!processedNames.has(server.name)) { - removeMCPServer(server.id); - } - } + serversToRemove.forEach((server) => removeMCPServer(server.id));Or ideally, add a batch removal method to the store for a single state update.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx
🧬 Code graph analysis (1)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (3)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsx (2)
MCPToolDisplay(7-12)MCPToolsList(21-117)apps/ui/src/store/app-store.ts (1)
useAppStore(1008-2937)libs/types/src/settings.ts (1)
MCPServerConfig(188-213)
🔇 Additional comments (3)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (3)
838-866: LGTM - Permission toggles now sync to server.The inline async handlers correctly update local state and persist to the server via
syncSettingsToServer(). This addresses the previous review feedback.
1026-1037: LGTM - Tools list rendering is properly guarded.The non-null assertion on
testState.tools!(line 1033) is safe because thehasToolscheck on line 1026 ensurestestState.toolsexists and has items.
1-48: LGTM - Imports follow coding guidelines.All imports correctly use shared packages (
@automaker/types) and path aliases (@/). The relative import for the siblingmcp-tools-listcomponent is appropriate.
Refactored 1348-line monolithic file into proper folder structure following folder-pattern.md conventions: Structure: - components/ - UI components (card, header, settings, warning) - dialogs/ - 5 dialog components (add/edit, delete, import, json edit) - hooks/use-mcp-servers.ts - all state management & handlers - types.ts, constants.ts, utils.tsx - shared code Main file reduced from 1348 to 192 lines (composition only). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-header.tsx (1)
39-47: Consider adding accessible label for icon-only button.The Refresh button displays only an icon without visible text. Adding an
aria-labelattribute would improve accessibility for screen reader users.🔎 Suggested enhancement
<Button size="sm" variant="ghost" onClick={onRefresh} disabled={isRefreshing} data-testid="refresh-mcp-servers-button" + aria-label="Refresh MCP servers" > <RefreshCw className={cn('w-4 h-4', isRefreshing && 'animate-spin')} /> </Button>apps/ui/src/components/views/settings-view/mcp-servers/dialogs/global-json-edit-dialog.tsx (1)
31-39: Consider simplifying the onOpenChange handler.The current logic only propagates
onOpenChangewhen opening, not when closing (which triggersonCancelinstead). While this works, it creates a subtle asymmetry. A cleaner approach might be:onOpenChange={(isOpen) => { if (!isOpen) onCancel(); onOpenChange(isOpen); }}However, if
onCancelalready handles settingopentofalsein the parent, this is fine as-is.apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (1)
119-132: Consider limiting parallel server tests on mount.All enabled servers are tested in parallel without concurrency control. With many servers, this could create a burst of simultaneous connections. Consider using a concurrency limit (e.g., 3-5 at a time) if users commonly configure many servers.
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/add-edit-server-dialog.tsx (1)
54-60: Consider adding form validation before save.The Save button is always enabled regardless of form state. Users can save servers with empty names or invalid configurations.
🔎 Proposed fix - Add basic validation
<Button onClick={onSave} + disabled={!formData.name.trim() || (formData.type === 'stdio' ? !formData.command.trim() : !formData.url.trim())} data-testid="mcp-server-save-button" >apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-card.tsx (1)
153-164: Non-null assertion is safe but could be cleaner.The
testState.tools!assertion on line 158 is guarded by thehasToolscheck on line 35, so it's safe. However, TypeScript could benefit from a narrower guard.🔎 Optional: Avoid non-null assertion with inline check
{hasTools && ( <CollapsibleContent> <div className="px-4 pb-4 pt-0 ml-7 overflow-hidden"> <div className="text-xs font-medium text-muted-foreground mb-2">Available Tools</div> - <MCPToolsList - tools={testState.tools!} - isLoading={testState.status === 'testing'} - error={testState.error} - className="max-w-full" - /> + {testState?.tools && ( + <MCPToolsList + tools={testState.tools} + isLoading={testState.status === 'testing'} + error={testState.error} + className="max-w-full" + /> + )} </div> </CollapsibleContent> )}apps/ui/src/components/views/settings-view/mcp-servers/dialogs/json-edit-dialog.tsx (2)
34-43: Redundant onOpenChange logic.The
onOpenChangehandler has asymmetric behavior: closing callsonCancel()but opening callsonOpenChange(open). Since opening would setopen=trueand the dialog is already open, this path is effectively unreachable. Consider simplifying.🔎 Simplified handler
<Dialog open={open} - onOpenChange={(open) => { - if (!open) { - onCancel(); - } else { - onOpenChange(open); - } - }} + onOpenChange={(isOpen) => { + if (!isOpen) onCancel(); + onOpenChange(isOpen); + }} >
65-72: Consider adding JSON syntax validation.The description states "Changes will be validated before saving" but the Save button only checks for non-empty input. Adding client-side JSON.parse validation would provide immediate feedback and prevent unnecessary round-trips.
🔎 Proposed validation
+const isValidJson = (value: string): boolean => { + try { + JSON.parse(value); + return true; + } catch { + return false; + } +}; // In the component: <Button onClick={onSave} - disabled={!jsonValue.trim()} + disabled={!jsonValue.trim() || !isValidJson(jsonValue)} data-testid="mcp-json-edit-save-button" >Consider also showing a validation error message when JSON is invalid.
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
143-147: Consider clearing deleteConfirmId after successful delete.The
onConfirmhandler callshandleDelete(deleteConfirmId)but the dialog state cleanup depends ononOpenChange. IfhandleDeletethrows or the deletion flow changes, the state could become inconsistent.🔎 Ensure state cleanup after delete
<DeleteServerDialog open={!!deleteConfirmId} onOpenChange={(open) => setDeleteConfirmId(open ? deleteConfirmId : null)} - onConfirm={() => deleteConfirmId && handleDelete(deleteConfirmId)} + onConfirm={() => { + if (deleteConfirmId) { + handleDelete(deleteConfirmId); + setDeleteConfirmId(null); + } + }} />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/ui/src/components/views/settings-view/mcp-servers/components/index.tsapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsxapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-card.tsxapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-header.tsxapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-tools-warning.tsxapps/ui/src/components/views/settings-view/mcp-servers/constants.tsapps/ui/src/components/views/settings-view/mcp-servers/dialogs/add-edit-server-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/delete-server-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/global-json-edit-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/import-json-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.tsapps/ui/src/components/views/settings-view/mcp-servers/dialogs/json-edit-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/hooks/index.tsapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/ui/src/components/views/settings-view/mcp-servers/index.tsapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsxapps/ui/src/components/views/settings-view/mcp-servers/types.tsapps/ui/src/components/views/settings-view/mcp-servers/utils.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/ui/src/components/views/settings-view/mcp-servers/components/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-card.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/delete-server-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/json-edit-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.tsapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-header.tsxapps/ui/src/components/views/settings-view/mcp-servers/constants.tsapps/ui/src/components/views/settings-view/mcp-servers/dialogs/add-edit-server-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/utils.tsxapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/components/views/settings-view/mcp-servers/index.tsapps/ui/src/components/views/settings-view/mcp-servers/dialogs/global-json-edit-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/ui/src/components/views/settings-view/mcp-servers/hooks/index.tsapps/ui/src/components/views/settings-view/mcp-servers/dialogs/import-json-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/types.tsapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-tools-warning.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-card.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/delete-server-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/json-edit-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.tsapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-header.tsxapps/ui/src/components/views/settings-view/mcp-servers/constants.tsapps/ui/src/components/views/settings-view/mcp-servers/dialogs/add-edit-server-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/utils.tsxapps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxapps/ui/src/components/views/settings-view/mcp-servers/index.tsapps/ui/src/components/views/settings-view/mcp-servers/dialogs/global-json-edit-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsapps/ui/src/components/views/settings-view/mcp-servers/hooks/index.tsapps/ui/src/components/views/settings-view/mcp-servers/dialogs/import-json-dialog.tsxapps/ui/src/components/views/settings-view/mcp-servers/types.tsapps/ui/src/components/views/settings-view/mcp-servers/components/mcp-tools-warning.tsx
🧠 Learnings (1)
📚 Learning: 2025-12-28T05:07:48.138Z
Learnt from: CR
Repo: AutoMaker-Org/automaker PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T05:07:48.138Z
Learning: Applies to apps/server/src/**/*.{ts,tsx} : Use `createEventEmitter()` from `lib/events.ts` for all server operations to emit events that stream to frontend via WebSocket
Applied to files:
apps/ui/src/components/views/settings-view/mcp-servers/hooks/index.ts
🧬 Code graph analysis (11)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/delete-server-dialog.tsx (2)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts (1)
DeleteServerDialog(2-2)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/json-edit-dialog.tsx (2)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts (1)
JsonEditDialog(4-4)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-header.tsx (2)
apps/ui/src/components/views/settings-view/mcp-servers/components/index.ts (1)
MCPServerHeader(1-1)apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/add-edit-server-dialog.tsx (1)
apps/ui/src/components/views/settings-view/mcp-servers/types.ts (2)
ServerFormData(5-14)ServerType(3-3)
apps/ui/src/components/views/settings-view/mcp-servers/utils.tsx (2)
apps/ui/src/components/views/settings-view/mcp-servers/constants.ts (1)
SENSITIVE_PARAM_PATTERNS(2-11)apps/ui/src/components/views/settings-view/mcp-servers/types.ts (2)
ServerType(3-3)ServerTestState(27-32)
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (1)
apps/ui/src/lib/utils.ts (1)
cn(5-7)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/global-json-edit-dialog.tsx (2)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts (1)
GlobalJsonEditDialog(5-5)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (3)
apps/ui/src/components/views/settings-view/mcp-servers/types.ts (4)
ServerFormData(5-14)defaultFormData(16-25)ServerTestState(27-32)ServerType(3-3)apps/ui/src/components/views/settings-view/mcp-servers/constants.ts (1)
MAX_RECOMMENDED_TOOLS(14-14)apps/ui/src/lib/http-api-client.ts (1)
getHttpApiClient(1304-1309)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/import-json-dialog.tsx (2)
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts (1)
ImportJsonDialog(3-3)apps/ui/src/components/ui/button.tsx (1)
Button(108-108)
apps/ui/src/components/views/settings-view/mcp-servers/types.ts (2)
apps/ui/src/components/views/settings-view/mcp-servers/index.ts (1)
MCPToolDisplay(2-2)apps/ui/src/components/views/settings-view/mcp-servers/mcp-tools-list.tsx (1)
MCPToolDisplay(7-12)
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-tools-warning.tsx (2)
apps/ui/src/components/views/settings-view/mcp-servers/components/index.ts (1)
MCPToolsWarning(3-3)apps/ui/src/components/views/settings-view/mcp-servers/constants.ts (1)
MAX_RECOMMENDED_TOOLS(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (21)
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-header.tsx (3)
1-3: LGTM!Imports are clean and follow the coding guidelines. Path aliases are used correctly.
5-23: LGTM!The interface is well-defined with clear, descriptive prop names and appropriate types. Component destructuring follows React best practices.
48-83: LGTM!The button logic and conditional rendering work correctly. Export and Edit JSON buttons are appropriately shown only when servers exist, and all event handlers are properly wired with good test IDs for testing.
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsx (1)
52-55: Same race condition concern applies here.Both toggle handlers share the same pattern. If the first comment's concern is valid, this handler has the same issue.
apps/ui/src/components/views/settings-view/mcp-servers/constants.ts (1)
2-11: LGTM with a minor note on broad patterns.The constants are well-defined. Note that
/auth/iis quite broad and will match substrings like "author" or "authenticate". If false positives become an issue, consider using word boundaries (e.g.,/\bauth\b/ior/[-_]?auth[-_]?/i).apps/ui/src/components/views/settings-view/mcp-servers/dialogs/delete-server-dialog.tsx (1)
17-41: LGTM!Clean, focused component with proper test IDs and clear responsibility separation. The parent component handles closing the dialog after confirmation.
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/global-json-edit-dialog.tsx (1)
70-77: LGTM - validation delegated to parent handler.The save button disables on empty input, and the parent's
handleSaveGlobalJsonEditperforms full JSON validation. This separation is reasonable.apps/ui/src/components/views/settings-view/mcp-servers/types.ts (1)
1-32: LGTM!Well-structured types for the MCP servers form and test state. The use of JSON strings for
headersandenvinServerFormDatais appropriate for form input handling, with parsing done at save time in the hook.apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts (3)
246-259: Argument splitting noted as intentional design choice.The
/\s+/splitting for args is documented in the PR as an intentional decision, with quoted-argument parsing deferred to a future update. This is acceptable for now.
1-10: LGTM - imports follow guidelines.Correctly uses
@/path aliases for internal imports and@automaker/typesfor shared package types, following the coding guidelines.
606-611: Iteration is safe;removeMCPServercorrectly follows immutable patterns.The loop iterates over a captured snapshot of
mcpServers. TheremoveMCPServerfunction creates a new array via.filter()and updates the store immutably withset(), ensuring the snapshot being iterated remains stable throughout the loop.apps/ui/src/components/views/settings-view/mcp-servers/hooks/index.ts (1)
1-1: LGTM!Clean barrel export for the hook.
apps/ui/src/components/views/settings-view/mcp-servers/index.ts (1)
1-2: LGTM!Clean barrel exports providing the public API for the MCP servers module.
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/add-edit-server-dialog.tsx (1)
1-21: LGTM - Clean imports and type definitions.The component properly imports from
@automaker/typesfor shared types as per coding guidelines, and uses relative imports for local types. The interface is well-defined with clear prop types.apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-card.tsx (1)
1-9: LGTM - Well-organized imports.Imports properly use
@automaker/typesfor shared types and local relative imports for module-specific types and utilities.apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-tools-warning.tsx (1)
1-25: LGTM - Clean warning component.Well-structured warning component with clear messaging and appropriate visual hierarchy. Uses the centralized
MAX_RECOMMENDED_TOOLSconstant properly.apps/ui/src/components/views/settings-view/mcp-servers/utils.tsx (1)
40-51: LGTM - Status icon mapping is clear and complete.The switch statement handles all defined status values and returns null for the 'idle' case via the default branch.
apps/ui/src/components/views/settings-view/mcp-servers/dialogs/import-json-dialog.tsx (1)
1-69: LGTM - Well-structured import dialog.The component follows established patterns with good UX (helpful placeholder, clear description about duplicate handling). Similar to the JSON edit dialog, consider adding client-side JSON validation for immediate user feedback, though this may be handled in
handleImportJson.apps/ui/src/components/views/settings-view/mcp-servers/dialogs/index.ts (1)
1-5: LGTM - Clean barrel exports.Standard barrel file pattern for dialog components.
apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx (2)
1-17: LGTM - Clean imports and component structure.Good use of barrel imports from
./hooks,./components, and./dialogs. The component is well-organized with clear separation between state management (hook) and presentation.
94-101: Permission changes are properly syncing to the server. TheMCPPermissionSettingscomponent callssyncSettingsToServer()after eachonAutoApproveChangeandonUnrestrictedChangecallback (lines 35 and 54 of mcp-permission-settings.tsx), ensuring persistence.Likely an incorrect or invalid review comment.
| onCheckedChange={async (checked) => { | ||
| onAutoApproveChange(checked); | ||
| await syncSettingsToServer(); | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the Zustand store persistence configuration
rg -n -A 15 "persist\(" apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 620
🏁 Script executed:
#!/bin/bash
# Find and inspect the syncSettingsToServer function
rg -n -B 2 -A 15 "syncSettingsToServer" apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsxRepository: AutoMaker-Org/automaker
Length of output: 1711
🏁 Script executed:
#!/bin/bash
# Look for the syncSettingsToServer function definition in the codebase
fd -e ts -e tsx | xargs rg -l "syncSettingsToServer" | head -10Repository: AutoMaker-Org/automaker
Length of output: 315
🏁 Script executed:
#!/bin/bash
# Check the structure of the Zustand store to understand how it persists
rg -n "automaker-storage" apps/ui/src/store/Repository: AutoMaker-Org/automaker
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Get the syncSettingsToServer implementation
cat -n apps/ui/src/hooks/use-settings-migration.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 4129
🏁 Script executed:
#!/bin/bash
# Check if there's more content in the use-settings-migration.ts file
wc -l apps/ui/src/hooks/use-settings-migration.tsRepository: AutoMaker-Org/automaker
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Get the syncSettingsToServer function
rg -n -A 30 "^export.*syncSettingsToServer" apps/ui/src/hooks/use-settings-migration.tsRepository: AutoMaker-Org/automaker
Length of output: 1319
🏁 Script executed:
#!/bin/bash
# Get lines around syncSettingsToServer function
sed -n '150,250p' apps/ui/src/hooks/use-settings-migration.tsRepository: AutoMaker-Org/automaker
Length of output: 3458
🏁 Script executed:
#!/bin/bash
# Find where MCPPermissionSettings is used
rg -n "MCPPermissionSettings" apps/ui/src/components/ | grep -v "export function MCPPermissionSettings"Repository: AutoMaker-Org/automaker
Length of output: 694
🏁 Script executed:
#!/bin/bash
# Search for the parent component that provides these callbacks
rg -n "onAutoApproveChange\|onUnrestrictedChange" apps/ui/src/components/ -B 3 -A 3Repository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Get the mcp-servers-section.tsx file to see how callbacks are defined
cat -n apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsx | head -150Repository: AutoMaker-Org/automaker
Length of output: 5291
🏁 Script executed:
#!/bin/bash
# Check if there's more content in mcp-servers-section.tsx
wc -l apps/ui/src/components/views/settings-view/mcp-servers/mcp-servers-section.tsxRepository: AutoMaker-Org/automaker
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Find the useMCPServers hook to understand how setMcpAutoApproveTools works
cat -n apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts | head -100Repository: AutoMaker-Org/automaker
Length of output: 4540
🏁 Script executed:
#!/bin/bash
# Check the full size of use-mcp-servers.ts
wc -l apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.tsRepository: AutoMaker-Org/automaker
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Find setMcpAutoApproveTools and setMcpUnrestrictedTools definitions in the store
rg -n "setMcpAutoApproveTools\|setMcpUnrestrictedTools" apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Get those lines with context to see how they're implemented
sed -n '1300,1400p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 3444
🏁 Script executed:
#!/bin/bash
# Search for mcpAutoApproveTools in the entire store file to find where it's defined
rg -n "mcpAutoApproveTools" apps/ui/src/store/app-store.ts | head -20Repository: AutoMaker-Org/automaker
Length of output: 372
🏁 Script executed:
#!/bin/bash
# Search for setMcp with a pattern to be more flexible
rg -n "setMcp" apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 314
🏁 Script executed:
#!/bin/bash
# Get the implementation of setMcpAutoApproveTools
sed -n '1618,1630p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 617
🏁 Script executed:
#!/bin/bash
# Also check if there's a call to syncSettingsToServer within the store action
sed -n '1615,1635p' apps/ui/src/store/app-store.tsRepository: AutoMaker-Org/automaker
Length of output: 984
Remove the redundant syncSettingsToServer() call and properly await the state update.
The setMcpAutoApproveTools action already calls syncSettingsToServer() internally, so calling it again in the component handler is redundant. Additionally, since onAutoApproveChange is async, it should be awaited:
onCheckedChange={async (checked) => {
await onAutoApproveChange(checked);
}}Remove the second await syncSettingsToServer() call—the sync is already handled by the store action.
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-permission-settings.tsx
around lines 33 to 36, the handler calls syncSettingsToServer() redundantly and
does not await the async store action; update the onCheckedChange handler to
await onAutoApproveChange(checked) and remove the separate await
syncSettingsToServer() call since setMcpAutoApproveTools/onAutoApproveChange
already performs the sync internally.
| onAdd, | ||
| }: MCPServerHeaderProps) { | ||
| return ( | ||
| <div className="p-6 border-b border-border/50 bg-linear-to-r from-transparent via-accent/5 to-transparent"> |
There was a problem hiding this comment.
Fix invalid Tailwind CSS class name.
The class bg-linear-to-r is not valid Tailwind CSS syntax. It should be bg-gradient-to-r to create a linear gradient background.
🔎 Proposed fix
- <div className="p-6 border-b border-border/50 bg-linear-to-r from-transparent via-accent/5 to-transparent">
+ <div className="p-6 border-b border-border/50 bg-gradient-to-r from-transparent via-accent/5 to-transparent">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="p-6 border-b border-border/50 bg-linear-to-r from-transparent via-accent/5 to-transparent"> | |
| <div className="p-6 border-b border-border/50 bg-gradient-to-r from-transparent via-accent/5 to-transparent"> |
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-header.tsx
around line 25, the Tailwind class name "bg-linear-to-r" is invalid; replace it
with the correct "bg-gradient-to-r" to enable a linear gradient background,
keeping the rest of the class list unchanged.
| <div className="flex items-center justify-between"> | ||
| <div> | ||
| <div className="flex items-center gap-3 mb-2"> | ||
| <div className="w-9 h-9 rounded-xl bg-linear-to-br from-brand-500/20 to-brand-600/10 flex items-center justify-center border border-brand-500/20"> |
There was a problem hiding this comment.
Fix invalid Tailwind CSS class name.
The class bg-linear-to-br is not valid Tailwind CSS syntax. It should be bg-gradient-to-br to create a diagonal gradient background.
🔎 Proposed fix
- <div className="w-9 h-9 rounded-xl bg-linear-to-br from-brand-500/20 to-brand-600/10 flex items-center justify-center border border-brand-500/20">
+ <div className="w-9 h-9 rounded-xl bg-gradient-to-br from-brand-500/20 to-brand-600/10 flex items-center justify-center border border-brand-500/20">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="w-9 h-9 rounded-xl bg-linear-to-br from-brand-500/20 to-brand-600/10 flex items-center justify-center border border-brand-500/20"> | |
| <div className="w-9 h-9 rounded-xl bg-gradient-to-br from-brand-500/20 to-brand-600/10 flex items-center justify-center border border-brand-500/20"> |
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/mcp-servers/components/mcp-server-header.tsx
around line 29, the Tailwind class `bg-linear-to-br` is invalid; replace it with
the correct `bg-gradient-to-br` to enable the intended diagonal gradient
background and keep the rest of the class list unchanged.
|
|
||
| const json = JSON.stringify({ mcpServers: exportData }, null, 2); | ||
| navigator.clipboard.writeText(json); | ||
| toast.success('Copied to clipboard'); | ||
| }; |
There was a problem hiding this comment.
Add error handling for clipboard API.
navigator.clipboard.writeText() can throw if clipboard access is denied or the API is unavailable. Wrap in try/catch to avoid uncaught exceptions.
🔎 Proposed fix
const handleExportJson = () => {
// ... build exportData ...
const json = JSON.stringify({ mcpServers: exportData }, null, 2);
- navigator.clipboard.writeText(json);
- toast.success('Copied to clipboard');
+ navigator.clipboard.writeText(json).then(
+ () => toast.success('Copied to clipboard'),
+ (err) => toast.error('Failed to copy: ' + (err instanceof Error ? err.message : 'Clipboard unavailable'))
+ );
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const json = JSON.stringify({ mcpServers: exportData }, null, 2); | |
| navigator.clipboard.writeText(json); | |
| toast.success('Copied to clipboard'); | |
| }; | |
| const json = JSON.stringify({ mcpServers: exportData }, null, 2); | |
| navigator.clipboard.writeText(json).then( | |
| () => toast.success('Copied to clipboard'), | |
| (err) => toast.error('Failed to copy: ' + (err instanceof Error ? err.message : 'Clipboard unavailable')) | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In
apps/ui/src/components/views/settings-view/mcp-servers/hooks/use-mcp-servers.ts
around lines 390 to 394, the call to navigator.clipboard.writeText(json) is
unguarded and can throw if the Clipboard API is unavailable or permission is
denied; wrap the clipboard write in a try/catch, await the promise, and on
success call toast.success('Copied to clipboard'), and on failure call
toast.error with a helpful message (and optionally console.error the error).
Also check for navigator.clipboard existence before calling and fall back to an
alternative (e.g., creating a temporary textarea and using
document.execCommand('copy')) or show a specific error toast if clipboard
support is missing.
| } catch { | ||
| // If URL parsing fails, try simple regex replacement for common patterns | ||
| return url.replace( | ||
| /([?&])(api[-_]?key|auth|token|secret|password|credential)=([^&]*)/gi, | ||
| '$1$2=***' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fallback regex doesn't match all sensitive patterns.
The fallback regex on lines 28-30 only covers a subset of SENSITIVE_PARAM_PATTERNS. For example, api[-_]?token, bearer, and credential patterns from constants aren't fully matched in the fallback, leading to inconsistent masking for invalid URLs.
🔎 Proposed fix - Align fallback with constants
} catch {
// If URL parsing fails, try simple regex replacement for common patterns
return url.replace(
- /([?&])(api[-_]?key|auth|token|secret|password|credential)=([^&]*)/gi,
+ /([?&])(api[-_]?key|api[-_]?token|auth|token|secret|password|credential|bearer)=([^&]*)/gi,
'$1$2=***'
);
}Alternatively, consider generating the fallback regex dynamically from SENSITIVE_PARAM_PATTERNS to keep them in sync.
🤖 Prompt for AI Agents
In apps/ui/src/components/views/settings-view/mcp-servers/utils.tsx around lines
26 to 32, the catch-block fallback regex only masks a subset of sensitive
parameter patterns and therefore misses items like api[-_]?token, bearer, and
others from SENSITIVE_PARAM_PATTERNS; update the fallback to build its regex
dynamically from the SENSITIVE_PARAM_PATTERNS constant (escape each pattern for
regex, join with | into a single group, and apply the same flags and delimiters
used elsewhere) so the fallback behavior matches the canonical masking rules for
all listed sensitive patterns.
- Restricted CORS to localhost origins to prevent remote code execution (RCE) attacks. - Updated MCP server configuration handling to enforce security warnings when adding or importing servers. - Introduced a SecurityWarningDialog to inform users about potential risks associated with server commands and configurations. - Ensured that only serverId is accepted for testing server connections, preventing arbitrary command execution. These changes improve the overall security posture of the MCP server management and usage.
|
making small tweaks then will merge |
Phase 1 complete - Document fixes for 3 MCP server bugs: - Bug AutoMaker-Org#1: JSON format inconsistency (array vs object, IDs lost) - Bug AutoMaker-Org#2: HTTP error handling (cryptic error messages) - Bug AutoMaker-Org#3: Race condition in auto-test (server not found) All bugs validated with multiple MCP server types (stdio + HTTP). Patches available for re-application after upstream updates. Related to upstream PR AutoMaker-Org#286 (MCP server support).
3 participants
Summary
Add Model Context Protocol (MCP) server integration to extend AI agent capabilities with external tools. Users can configure MCP servers (stdio, SSE, HTTP) in global settings.
Note: MCP servers are currently configured globally. Per-project configuration is planned for a future update.
Test plan
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.